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

Fix Iframe loading #31973

Open
gterzian opened this issue Apr 2, 2024 · 2 comments
Open

Fix Iframe loading #31973

gterzian opened this issue Apr 2, 2024 · 2 comments
Labels
A-content/script Related to the script thread

Comments

@gterzian
Copy link
Member

gterzian commented Apr 2, 2024

As part of debugging at new failing test in #31505, I noted some(known?) problems with how iframes are loaded.

The test in question is /html/browsers/browsing-the-web/navigating-across-documents/initial-empty-document/load-pageshow-events-iframe-contentWindow.html, which asserts that no load events are fired for an iframe which has no navigable("load" & "pageshow" events do not fire on contentWindow of iframe that stays on the initial empty document).

We fail all tests, meaning we fire load events, for all subtests, with the exception of "load & pageshow events do not fire on contentWindow of <iframe> element created with src=''". The test fails now also with #31505, hence my investigation, but I think I may want to let it fail with all others, and instead fix all tests in one go.

Quick recap on how this relates to the spec:

  1. Entry point for processing are iframe HTML element insertion steps, which call into process the iframe attributes.
  2. We then hit the "2 otherwise"(meaning it has a url), which then either:
  3. Immediately returns if there is no src, which means not firing any load event.
  4. Immediately run the load steps for an "about:blank" iframe, which means firing a load event on the element, not the content window
  5. Navigate the iframe otherwise, which means eventually firing the load event on the content window as per completely-finish-loading

So, in other words, there are three:

  • in the case of no src, no event should fire at all, and no navigation should happen.
  • in the case of "about:blank", a load event should synchronously fire on the element, but no navigation should happen.
  • Otherwise, we should navigate the iframe, and then only fire the load event when the load completes(so that's asynchronous).

What we are currently doing, and why it is wrong:

bind_to_tree is the entry point, where we do two things via a delayed task:

  1. create_nested_browsing_context
  2. process_the_iframe_attributes(ProcessingMode::FirstTime).

Both end-up calling into start_new_pipeline(with the exception of the no src case, which returns), where 1 uses PipelineType::InitialAboutBlank, and 2 PipelineType::Navigation.

PipelineType::InitialAboutBlank results in a kind of synchronous loading of the iframe doc via ScriptThread::process_attach_layout, which eventually calls into document.maybe_queue_document_completion(), which then sends a msg to the constellation by way of document.notify_constellation_load. It does not end there: the constellation will then respond with a DispatchIFrameLoadEvent message, which will then result in calling the iframe load event steps.

PipelineType::Navigation starts with a ScriptMsg::ScriptLoadedURLInIFrame sent to the constellation, resulting in what I assume is a "normal" navigation workflow.

This is wrong because we are:

  1. For an about:blank, firing the load event on the element asynchronously.
  2. In all cases(except the early return no src) doing a navigation, which results in firing the load event on the document.

So somehow we need to restructure this, where:

  1. When binding to the tree
  2. If url is null, do nothing(or send some new message to the constellation that deals with it, just noting a new empty pipeline matching a src-less iframe)
  3. If url is about:blank, synchronously run the load steps, and then send the PipelineType::InitialAboutBlank msg.
  4. Otherwise, send the PipelineType::Navigation message. But probably this require doing some setup for the pipeline that is currently done with PipelineType::InitialAboutBlank.
@gterzian gterzian added the A-content/script Related to the script thread label Apr 2, 2024
@mrobinson
Copy link
Member

@gterzian Very nice exploration! Do you know if this could be causing the thing where we sometimes see two load events when loading about: blank.

@gterzian
Copy link
Member Author

gterzian commented Apr 3, 2024

Very nice exploration!

Thanks!

Do you know if this could be causing the thing where we sometimes see two load events when loading about: blank.

Yes I think so, although there are so many details that it needs some further investigation, but I think if we load about: blank we can get two load events: one from the first "synchronous" creation of the BC via ScriptThread::process_attach_layout, and one from the subsequent navigation of that BC(since the Iframe "bind to tree" steps start the two workflows).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-content/script Related to the script thread
Projects
None yet
Development

No branches or pull requests

2 participants