Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should location-object-navigate (and friends) queue a task to navigate instead of navigating directly? #3730

Closed
zetafunction opened this issue Jun 1, 2018 · 11 comments · Fixed by #6315
Labels
interop Implementations are not interoperable with each other topic: javascript: URLs topic: location topic: navigation

Comments

@zetafunction
Copy link

location-object-navigate doesn't currently specify that a task is queued to perform the navigation. However, it looks like some browsers do queue this:

document.body.appendChild(document.createElement('iframe'));window[window.length-1].location = 'javascript:console.log(2);';console.log(1);

In IE, this prints 2 and then 1.
In Chrome, Firefox, and Safari, this prints 1 and then 2.

Similarly, should window.open() queue a task to navigate as well?

var w = window.open('javascript:window.opener.console.log(2)');console.log(1);

In Chrome and Safari, this prints 2 and then 1.
In Firefox and IE, this prints 1 and then 2.

And there's also HTMLIFrameElement.src:

document.body.appendChild(document.createElement('iframe')).src = 'javascript:console.log(2);';console.log(1);

In Chrome, IE, and Safari, this prints 2 and then 1.
In Firefox, this prints 1 and then 2.

Queueing a task would be consistent with https://html.spec.whatwg.org/multipage/links.html#following-hyperlinks-2, but it's not clear to me it's the right thing to do.

I'm not sure of a way to test this hypothesis without using javascript: URLs: it's entirely possible that the reason Firefox always prints 1 before 2 is because it is the only implementation that queues a task to process javascript: URLs.

@annevk
Copy link
Member

annevk commented Jun 2, 2018

How would it throw an exception if it queued a task?

@zetafunction
Copy link
Author

Doesn't the issue of throwing exceptions also apply to hyperlink navigations? So following that line of reasoning, then maybe nothing should be queued?

@annevk
Copy link
Member

annevk commented Jun 2, 2018

No, that doesn't set the exceptions enabled flag.

@gterzian
Copy link
Member

gterzian commented May 14, 2019

Is the below test correct, if we assume that no task should be queued(cc servo/servo#23368)?

https://github.com/web-platform-tests/wpt/blob/bb11ac51f133591d7759c991ed822b02db05f897/html/semantics/scripting-1/the-script-element/execution-timing/028.html

This test uses location, and expects the second log to happen before the JS url execution:

log('inline script #1');
window.location.replace('javascript:log(\'JS URL\')');
log('end script #1');

Wouldn't this only happen if a task was queued? (Or is the expected delay related to execution of the newly created script?)

@gterzian
Copy link
Member

gterzian commented May 14, 2019

Also, just to point it out, in the case of the anchor element, following-hyperlinks says to "queue a task to navigate".

Then, inside navigate, we are told to queue a task to execute a JS url request.

So this means that in the examples discussed above, while no task should be queued to navigate(which includes unloading and so on), a task should be queued to perform step 13, in the case that it involves a JS url.

So the test I mentioned above,/html/semantics/scripting-1/the-script-element/execution-timing/028.html is probably "correct" in that it doesn't rely on the navigation itself queuing as task, I think, just step 13 of it.

On the other hand, related the anchor element, there is also /html/semantics/scripting-1/the-script-element/execution-timing/029.html. That test somehow seems to expect that a task is queued for the navigation, which matches the spec(since anchor element queue tasks to follow their hyperlinks), however the test seems to also expect that the JS url is processed as part of that same "navigate task", not as a subsequent task queued as part of navigation. Or at least that is what I encountered when working on this at servo/servo#23368. I tried following the spec "to the letter" and actually doing the navigation in a task, with the JS url done in a task queued by the navigation task, but that resulted in the JS url executing only after the load event fired(so "JS URL" was not even in the list of events logged in the test).

So the test suite seems to expect that when navigate is done in a task(in the case of an anchor following a hyperlink), no task is enqueued for the JS url part, and when navigate is not done in a task, a task is enqueued for the JS url part.

I haven't looked into window.open yet, but I assume it's a similar situation. Perhaps no task is queued for the navigation, but a task is queued by some engines to execute the JS url.

Since executing the JS url involves manipulating the realm of JS objects(I think), it makes sense to do thart part in a task on the event-loop. Note that otherwise step 13 is done "in-parallel".

@gterzian
Copy link
Member

Update: In the context of window.open, if you don't queue a task for thh JS url exec, you get an error with this test because this.checkValues is not a function inside the JS url passed to open.

That's because the tests only sets checkValues on the return value of open after the call to open. So the test seems to strongly expect that the JS url is only executed in a subsequent task, so that the current task gets a change to set checkValues.

@gterzian
Copy link
Member

gterzian commented May 14, 2019

So all in all, I'd go for the following:

  1. It's right to queue a task to execute a JS url in all cases, except maybe following hyperlinks.
  2. It's weird to queue a task to execute a JS url in following hyperlinks, since that means the JS url is only executed after at least two "turns of the loop", since the initial navigation steps are already done in a task. /html/semantics/scripting-1/the-script-element/execution-timing/029.html seems to expect that only one task is queued to do the navigation, and execute the JS url in that same task.
  3. If you try never enqueuing a task to execute the JS url(but still do the navigation of following a hyperlink inside a task), /execution-timing/029.html passes, however a few other test fail, including on unrelated test that is using window.open. So since more tests fail, and even seemingly unrelated ones, I'd say that this probably isn't the right approach.

So at this point the spec could perhaps say "unless this navigation is the result of running the "following-hyperlinks" algorithm, in which case the following steps should be done in the same task that invoked the navigation steps? Would need a bit of reshuffle at step 12 too, so that it doesn't read "continue running these steps in parallel" in that particular case...

@bzbarsky
Copy link
Contributor

there is also /html/semantics/scripting-1/the-script-element/execution-timing/029.html

This test is, as you note, not following the spec. Note that it's marked as failing in Firefox... It also fails in Safari, per https://wpt.fyi/results/html/semantics/scripting-1/the-script-element/execution-timing/029.html?label=master&product=chrome%5Bexperimental%5D&product=edge&product=firefox%5Bexperimental%5D&product=safari%5Bexperimental%5D&aligned&q=html%2Fsemantics%2Fscripting-1%2Fthe-script-element%2Fexecution-timing%2F029.html

It seems to be passing in Chrome for reasons that are entirely unclear to me, but Chrome is making a bunch of changes in this area right now to align with the spec, so I suspect the right thing is to just fix this test to follow the spec.

That said, last I checked there were suggestions that the spec (and implementations) be changed to not queue an extra task when following hyperlinks. I do NOT thing we should add special-casing in navigation for the hyperlink case.

@gterzian
Copy link
Member

gterzian commented May 14, 2019

Ok, thanks for the info, I will happily mark /execution-timing/029.html as failing in Servo too, and proceed with following the current spec to the letter..

Actually I'll look at changing the test instead...

@annevk
Copy link
Member

annevk commented May 14, 2019

What @bzbarsky said. I think navigation (and form submission too as a special case) should be invoked synchronously and not involve additional tasks that can result in changing state and such.

@domenic domenic added the interop Implementations are not interoperable with each other label Aug 8, 2022
domenic added a commit to web-platform-tests/wpt that referenced this issue Oct 10, 2022
domenic added a commit to web-platform-tests/wpt that referenced this issue Oct 10, 2022
@domenic
Copy link
Member

domenic commented Oct 10, 2022

Update: in #6315 we're speccing navigation to generally start synchronously. That is, neither location.href nor following hyperlinks will inherently queue a task.

However, javascript: URL navigation is always done in a task. So there will always be a single task for javascript: URL navigation.

Tests are added for that in web-platform-tests/wpt#36358, and at least Firefox and Chrome pass them.

I will also look into fixing the 029.html test.

domenic added a commit to web-platform-tests/wpt that referenced this issue Oct 10, 2022
See some discussion in whatwg/html#3730 about this test. However, it is very broken, and when we remove the brokenness, it becomes redundant with the just-added javascript-url-task-queuing.html. In particular, it's assuming beforeunload will fire. But beforeunload will never fire for a javascript: URL that returns a non-string value, since we're never in danger of unloading the document.
domenic added a commit to web-platform-tests/wpt that referenced this issue Oct 10, 2022
domenic added a commit to web-platform-tests/wpt that referenced this issue Oct 10, 2022
See some discussion in whatwg/html#3730 about this test. However, it is very broken, and when we remove the brokenness, it becomes redundant with the just-added javascript-url-task-queuing.html. In particular, it's assuming beforeunload will fire. But beforeunload will never fire for a javascript: URL that returns a non-string value, since we're never in danger of unloading the document.
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Oct 21, 2022
…ng, a=testonly

Automatic update from web-platform-tests
Add tests for javascript: URL task queuing

See whatwg/html#3730. Follows the spec in whatwg/html#6315.

--
Delete failing-in-all-browsers 029.html

See some discussion in whatwg/html#3730 about this test. However, it is very broken, and when we remove the brokenness, it becomes redundant with the just-added javascript-url-task-queuing.html. In particular, it's assuming beforeunload will fire. But beforeunload will never fire for a javascript: URL that returns a non-string value, since we're never in danger of unloading the document.

--

wpt-commits: b9d6751e534aed6a8d9eb1e7e51640f968b943bb, 1a2f69d137c5ccbaaf678bf372a1a410ced97eed
wpt-pr: 36358
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Oct 26, 2022
…ng, a=testonly

Automatic update from web-platform-tests
Add tests for javascript: URL task queuing

See whatwg/html#3730. Follows the spec in whatwg/html#6315.

--
Delete failing-in-all-browsers 029.html

See some discussion in whatwg/html#3730 about this test. However, it is very broken, and when we remove the brokenness, it becomes redundant with the just-added javascript-url-task-queuing.html. In particular, it's assuming beforeunload will fire. But beforeunload will never fire for a javascript: URL that returns a non-string value, since we're never in danger of unloading the document.

--

wpt-commits: b9d6751e534aed6a8d9eb1e7e51640f968b943bb, 1a2f69d137c5ccbaaf678bf372a1a410ced97eed
wpt-pr: 36358
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interop Implementations are not interoperable with each other topic: javascript: URLs topic: location topic: navigation
Development

Successfully merging a pull request may close this issue.

6 participants