<iframe src="javascript:..."> executes too soon, which is a symptom of HTMLIFrameElement::navigate_or_reload_child_browsing_context needing to perform navigation steps at the right time/on the right thread

[pshaughn]

WPT tests html/semantics/scripting-1/the-script-element/execution-timing/040, 080, 108, and 109 all create iframes with a javascript: url as their src, and in each case the javascript starts executing immediately, before the task that created the iframe finishes. According to these tests, which other browsers pass, that's incorrect behavior.

[pshaughn]

109 also fails to fire an onload event: that could be directly related to this, since if javascript has already started running Servo might assume it is too late for onload.

[gterzian]

I think this could be fixed at

servo/components/script/dom/htmliframeelement.rs

Line 140 in bd0ae59

if load_data.url.scheme() == "javascript" {

by doing the same thing as is done when a non-iframe browsing-context navigates to a javascript url at

servo/components/script/script_thread.rs

Line 980 in 5f1681a

let task = task!(navigate_javascript: move || {

Spec: https://html.spec.whatwg.org/multipage/#navigating-across-documents:javascript-protocol

[pshaughn]

@highfive assign me

[highfive]

Hey @pshaughn! Thanks for your interest in working on this issue. It's now assigned to you!

[pshaughn]

I'm having trouble with task queuing; .queue is giving me some compile errors "std::cell::UnsafeCell<...> cannot be shared between threads safely" with a couple different types inside. I am guessing this implies I need to do some sort of explicit handoff of DOMRefCells into the thread that'll be running the task, but I don't understand how to write that.

[jdm]

In general that usually happens when there is a DomRoot or Dom variable declared outside of the task! macro, and then used inside of it. When that's the case, you need to declare a Trusted value and use that from inside the task (calling the root() method in order to obtain a DomRoot again).

[jdm]

If you're using self inside of the task, that could also cause it for the same reason, and you would need to create a Trusted value that encompasses self and roots it inside of the task.

[pshaughn]

I've successfully put that JS eval and the following update to self.load_blocker in a task (pshaughn@ca40006) , and I'm running the WPT execution-timing tests now. 108 has gone to PASS, but 035 has gone from PASS to FAIL, and 040, 080, and 109 are still failing.

The code here isn't commented with specification algorithm steps and it's not obvious how it corresponds. I'm now going to step back from making functional changes here and spend some time commenting and possibly refactoring.

[jdm]

That sounds like a valuable improvement!

[pshaughn]

I've done some step renumbering and TODO-noting so far in pshaughn@8c42b83 but I haven't yet figured out which implementation of the numbered navigation steps the iframe case ends up using. There's nothing worth a PR yet here.

[pshaughn]

This seems to be one of several things that needs to happen in the course of a bigger overhaul, along with #25098. Right now iframe navigation is missing many specified steps. It's unclear which thread needs to do which steps, but conceptually navigate_or_reload_child_browsing_context should be causing all of Window::load_url steps 1-14 to happen in a way that looks synchronous to the caller, including beforeunload firing, and then causing step 16 (which includes this) to happen asynchronously after all of those. This probably intersects #25352, since following a hyperlink with an iframe target also needs to go through a codepath that isn't Window::load_url but does steps 1-14 of it.

[gterzian]

Oh yeah, so to come back to my earlier comment re:

servo/components/script/dom/htmliframeelement.rs

Line 140 in bd0ae59

if load_data.url.scheme() == "javascript" {

It looks like we shouldn't be running the Javascript for a JS url at that point at all, and certainly not in the context of the document containing the iframe, rather it should be run as part of https://html.spec.whatwg.org/multipage/#javascript-protocol in the context of the document of the nested browsing context(the one inside the iframe).

It's unclear which thread needs to do which steps, but conceptually navigate_or_reload_child_browsing_context should be causing all of Window::load_url steps 1-14 to happen in a way that looks synchronous to the caller, including beforeunload firing, and then causing step 16 (which includes this) to happen asynchronously after all of those

I would say when the spec says to "Navigate the element's nested browsing context to resource."(for example at https://html.spec.whatwg.org/multipage/#otherwise-steps-for-iframe-or-frame-elements), we should do the whole thing "async" from the perspective of the document containing the iframe, by sending a message to the constellation, which would then forward it to the appropriate script-thread(which could be the same as where the message came from), and in response to that message, window.load_url should be called on the window of the nested browsing context.

So I'd say that ScriptMsg::ScriptLoadedURLInIFrame, instead of starting a custom workflow loading a new page, should instead result in the constellation sending a message to the script-thread in wich the nested BC is found, and the script-thread should call window.load_url on it, bringing the navigating of the nested BC that occurs when the iframe attributes are manipulated or initially set in line with the navigation that would occur "internally" if the nested BC would cause itself to navigate.

[gterzian]

Actually, in the light of https://html.spec.whatwg.org/multipage/browsers.html#allowed-to-navigate I think one can only successfully set the src of an iframe if it's running in the same script-thread. In that case we could either abort if the nested BC cannot be navigated, and otherwise we could simply get the right window directly via the local scrip-thread, and call load_url on it(and forget about the constellation in that workflow).

I think we currently always create a new pipeline via ScriptMsg::ScriptLoadedURLInIFrame, potentially in a new process/script-thread if it's cross-site, and unload the previous pipeline(running in another script-thread). That doesn't look compliant with the concept of "allowed-to-navigate".

[pshaughn]

If passing the allowed_to_navigate check really does imply a guarantee that the caller, the old document, and the new document will all be on the same script thread, then that simplifies things considerably.

[jdm]

@gterzian I think you're misreading that algorithm - in the case of the top-level browsing context for joshmatthews.net containing an iframe for github.com, it is perfectly valid for the top level context to set the iframe's src value to facebook.com. This falls under step 4 of that algorithm, since the conditions for the first three steps do not hold.

[gterzian]

@jdm Ok, thanks for checking.

So then I think the workflow described above involving the constellation and eventually calling window.load_url on the nested BC in whatever script-thread it is running, would seem like the way forward.

One could still combine that with an initial check to see if the nested BC is in the same script-thread, giving something like:

1. When navigating a nested BC as a result of processing the iframe attributes, then
2. First use ScriptThread. documents.find_window to see if the nested BC is running in the same script-thread, if so, call window.load_url on it.
3. Otherwise, send a message to the constellation, similar to ScriptMsg::ScriptLoadedURLInIFrame, but instead of immediately creating a new pipeline, the constellation would send a message to the script-thread where the nested BC is running, and that script-thread would, when receiving the message, call window.load_url.

In both cases, I'm not sure when exactly the src attribute of the iframe should return the "new" url when accessed. For example if the navigation doesn't go through, but that only becomes clear in a part of the algorithm running in the other script-thread.

[jdm]

Since the src IDL attribute only reflects the content attribute, we do not need any special logic around what to return.

[pshaughn]

I like the idea of checking if we're on the same script thread and doing all the navigation checks synchronously in the common case that we are. Whether or not beforeunload has been called yet is observable in the same-script-thread case, since the beforeunload handler might itself do something observable, so it'll be easier to get correctness there if happens in sync.