Navigation API: fix an ordering issue#11512
Conversation
Because navigatesuccess/navigateerror events can run JavaScript code, they can change the "ongoing API method tracker" pointer. Thus, it's better to resolve/reject the finished promise for the API method tracker before firing the event. This does not change the observable order in normal cases: it remains the case that JavaScript event handlers for navigatesuccess/navigateerror run before promise reactions, because promise resolution is always delayed by a microtask. However, it fixes some tricky reentrant cases.
annevk
left a comment
There was a problem hiding this comment.
This ordering seems like something we should stick with everywhere, though I suppose it really only manifests here because the resolving/rejecting is conditional. But still, consistency is good.
|
This looks very reasonable. I wouldn't be surprised if we have the same bug as chromium here. |
|
The test in my CL (https://chromium-review.googlesource.com/c/chromium/src/+/6622087) fails the assertion at the start of the promote an upcoming API method tracker to ongoing steps. I haven't isolated a case where a promise resolves incorrectly or anything like that yet, but we definitely are violating spec assertions. |
|
I don't have official review permissions, but the spec diffs match what I expected based on my proposed code change. |
…success] Order was changed in whatwg/html#11512 (see that PR for details as to why). https://whatpr.org/html/10919/nav-history-apis.html#abort-a-navigateevent
…success] (#63377) Order was changed in whatwg/html#11512 (see that PR for details as to why). https://whatpr.org/html/10919/nav-history-apis.html#abort-a-navigateevent PR Close #63377
…success] (#63377) Order was changed in whatwg/html#11512 (see that PR for details as to why). https://whatpr.org/html/10919/nav-history-apis.html#abort-a-navigateevent PR Close #63377
Because navigatesuccess/navigateerror events can run JavaScript code, they can change the "ongoing API method tracker" pointer. Thus, it's better to resolve/reject the finished promise for the API method tracker before firing the event.
This does not change the observable order in normal cases: it remains the case that JavaScript event handlers for navigatesuccess/navigateerror run before promise reactions, because promise resolution is always delayed by a microtask. However, it fixes some tricky reentrant cases, which currently cause spec assertion failures.
This was discovered by @natechapin in https://chromium-review.googlesource.com/c/chromium/src/+/6622087, while he was implementing the fix from #11409. I'd like his review, as well as ideally @farre's.
I'm still working with Nate on getting clarity on an exact test case that exercises this bug, and what the before/after behavior is. It might be the test case already in https://chromium-review.googlesource.com/c/chromium/src/+/6622087; I'm not sure.Those tests will exercise this code path, and a spec assertion will get hit without this.(See WHATWG Working Mode: Changes for more details.)
/nav-history-apis.html ( diff )