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

Proposal: stop triggering navigations to about:blank on iframe insertion #6863

Closed
rakina opened this issue Jul 15, 2021 · 10 comments · Fixed by #6869
Closed

Proposal: stop triggering navigations to about:blank on iframe insertion #6863

rakina opened this issue Jul 15, 2021 · 10 comments · Fixed by #6869

Comments

@rakina
Copy link
Member

rakina commented Jul 15, 2021

(This was discussed a bit in #6491 (comment) but I'm making a separate issue since it's quite orthogonal, and since I just thought about a potential way forward on this)

Background
When an iframe is created/inserted it will navigate to the URL from its src attribute (if it's set and the srcdoc attribute is unset), replacing the initial empty document. If this URL is about:blank, in the current spec, it should still replace the initial empty document with a new about:blank document. I think this navigation is triggered in all browsers, but how it runs (synchronously/asynchronously) and its effect (removing the "initial about:blank document"-ness or not) differs (see next section). Also, when the src attribute is not set, according to the spec the iframe should stay on the initial empty document. However, at least in Chrome this actually triggers an about:blank navigation, as if the src is set to about:blank.

Behavior of the about:blank navigation on initial insertion

  • In Chrome and probably Safari, the navigation happens synchronously when the iframe is inserted. This means the initial empty document is immediately replaced by the non-initial about:blank. However, in both Chrome and Safari, the non-initial about:blank is actually treated the same way as the initial empty document. So, the end result is pretty much the same with not navigating at all (see WPT for initial empty document history replacement behaviors web-platform-tests/wpt#28541)
  • In Firefox, the navigation happens asynchronously - this means the initial empty document will be available for a while until it got replaced by the new about:blank document. Since it's asynchronous, the navigation can be canceled (e.g. by document.open(), as mentioned in this comment). After the navigation finished, the iframe will not show the initial empty document anymore.

Web-exposed behavior differences

  1. The "initial empty document"-ness differs across browsers as mentioned above. In Chrome & Safari, the new about:blank document will still be considered as the initial empty document, while in Firefox it's not. This has an impact on whether future navigations will replace that document or not. For more details see WPT for initial empty document history replacement behaviors web-platform-tests/wpt#28541 and Which navigations should add/replace new history entry after initial empty document? #6491.

  2. The document will be replaced asynchronously in Firefox, while it's replaced synchronously/immediately in Chrome and Safari. I think this is actually a very big difference, because in Chrome & Safari you can do this and the text will stay, while in Firefox it will be cleared when the new document committed:

 let f = document.createElement("iframe");
 frameContainer.appendChild(f);
 f.contentDocument.body.innerHTML = "some text";

Tester pages to compare in various browsers: iframe with no src, iframe with src="about:blank"

Proposal: stop triggering navigations to about:blank on iframe insertion
I propose that we stop triggering navigations to about:blank on iframe insertion, so that when an iframe with src set to about:blank is inserted, it will just stay on the initial empty document, similar to when src is not set (at least the spec version). My reasoning is as follows:

  • This is essentially what is already happening (probably unintentionally) in Chrome and Safari since both the "initial empty document"-ness and the document will be kept after the iframe is inserted.
  • Although the document is actually the new about:blank document committed synchronously, it's virtually indistinguishable from the initial empty document: the origin and other document properties should be the same, and it's not possible to get a reference to the initial empty document before insertion so nobody knows that it was ever there to begin with.
  • This is the same behavior as window.open() when the URL is not set, empty or is about:blank: instead of triggering a navigation, it will just fire the load event on the window (see test page).

Potential problems:

  • Firefox behavior will need to change to follow Chrome and Safari here. I think this is reasonable, especially the part where we stop changing the document asynchronously after potentially modifying it. I don't think Chrome and Safari can follow Firefox's behavior here without potentially breaking some websites that depend on being able to modify the iframe's document immediately after creation.
  • Events/side effects from navigations. I think since in Chrome and Safari the navigation was already synchronous to begin with, it's not possible to observe events/changes related to navigations on the document/window. For example, we can't add a load event listener to the contentWindow before insertion, and the load event would've already fired if we try to add it after insertion. I'm not sure if this has any effect on the load event on the <iframe> itself, but I think we can just fire the load event here too like the window.open() case above. CMIIW!
  • Keeping the "initial empty document" status means the next same-origin document will reuse the Window (see Rationalize behavior of Window object reuse #3267). I think this is already what's happening in Chrome and maybe Safari, I haven't checked though. Not really sure if this is bad, but since browsers already have support for this maybe it's not really a big deal?
  • [Maybe there are some subtleties I'm missing, please let me know if so!]

cc @domenic @annevk @zetafunction @ArthurSonzogni @csreis who might be interested.

PS: I think some Chrome folks were thinking of following Firefox's behavior previously to remove all cross-document synchronous commits in Chrome's renderer, but I recently found some tests in Chrome that do not expect the document to be changed asynchronously, and pretty worried that this exists in real websites, so I think it's probably best to just avoid this special navigation entirely, and it's pretty consistent with the window.open() case too!

@domenic
Copy link
Member

domenic commented Jul 15, 2021

I love this. The consistency with window.open() is great, and unifying the missing attribute/empty string/invalid URL/explicit about:blank cases makes so much more sense than the current situation where those diverge.

I think we should update the spec in this direction, and I agree with your assessment that it should match Chrome and Safari observable behavior---so we already have multi-implementer interest!

Regarding window object reuse, I think that's a separable interop problem; the tests at https://wpt.fyi/results/html/browsers/the-window-object/window-reuse-in-nested-browsing-contexts.tentative.html?label=master&label=experimental&aligned show we have some work to do there. I think it's best to just not touch that aspect of the spec as part of this change, although I agree it has some impact.

/cc @smaug---- as well.

I will try to work on a spec change for this. The load event might be tricky so that'll need careful review.

@rniwa
Copy link

rniwa commented Jul 16, 2021

@cdumez

domenic added a commit that referenced this issue Jul 16, 2021
Previously, we did a confusing thing where we would navigate to a non-initial about:blank. 2/3 engines instead do something observably equivalent to not navigating at all; they just fire a load event. This is much simpler, and matches window.open().

Closes #6863.
@smaug----
Copy link

How would this be consistent with window.open() ? (or perhaps browsers behave differently there too).

load event firing on iframe needs to be clarified. Is that happening synchronously or asynchronously?

And reusing the Window could be breaking change.

@domenic
Copy link
Member

domenic commented Aug 2, 2021

How would this be consistent with window.open() ? (or perhaps browsers behave differently there too).

Yes, it is consistent with how browsers behave on window.open(); see #6869 for the proposed change to both of them.

load event firing on iframe needs to be clarified. Is that happening synchronously or asynchronously?

It fires asynchronously. https://html.spec.whatwg.org/#completely-finish-loading remains unchanged for now.

@rakina, could you expand the tests at web-platform-tests/wpt#29745 to cover the iframe load event as well?

And reusing the Window could be breaking change.

Window reuse is a separate interop problem. It seems unlikely that pages are depending on any particular semantics given how non-interoperable that is. But I am happy to tackle that next if you're excited about aligning Gecko with the majority there.

@zetafunction
Copy link

However, in both Chrome and Safari, the non-initial about:blank is actually treated the same way as the initial empty document.

I don't think this is completely true. It definitely affects things like reuse of the Window object.

PS: I think some Chrome folks were thinking of following Firefox's behavior previously to remove all cross-document synchronous commits in Chrome's renderer, but I recently found some tests in Chrome that do not expect the document to be changed asynchronously, and pretty worried that this exists in real websites, so I think it's probably best to just avoid this special navigation entirely, and it's pretty consistent with the window.open() case too!

I wonder if the opposite problem might exist. Also see below.

Keeping the "initial empty document" status means the next same-origin document will reuse the Window (see Rationalize behavior of Window object reuse #3267). I think this is already what's happening in Chrome and maybe Safari, I haven't checked though. Not really sure if this is bad, but since browsers already have support for this maybe it's not really a big deal?

This is actually not the case in Chrome today: we will not reuse the Window object, since the synchronous (re-)navigation to about:blank commits something that is no longer considered the initial empty document. See https://crbug.com/778318

To be clear, I am not against trying to simplify things, but it will definitely have web-visible effects.

@rakina
Copy link
Member Author

rakina commented Aug 3, 2021

@rakina, could you expand the tests at web-platform-tests/wpt#29745 to cover the iframe load event as well?

I think this is already covered by this test, let me know if I need to add more!

PS: I think some Chrome folks were thinking of following Firefox's behavior previously to remove all cross-document synchronous commits in Chrome's renderer, but I recently found some tests in Chrome that do not expect the document to be changed asynchronously, and pretty worried that this exists in real websites, so I think it's probably best to just avoid this special navigation entirely, and it's pretty consistent with the window.open() case too!

I wonder if the opposite problem might exist. Also see below.

For the asynchronous -> synchronous / "completely remove" change, I think it's less of a problem, see this comment.

Keeping the "initial empty document" status means the next same-origin document will reuse the Window (see Rationalize behavior of Window object reuse #3267). I think this is already what's happening in Chrome and maybe Safari, I haven't checked though. Not really sure if this is bad, but since browsers already have support for this maybe it's not really a big deal?

This is actually not the case in Chrome today: we will not reuse the Window object, since the synchronous (re-)navigation to about:blank commits something that is no longer considered the initial empty document. See https://crbug.com/778318

To be clear, I am not against trying to simplify things, but it will definitely have web-visible effects.

Chatted a bit more with @zetafunction today, and looks like this is true at least in Chrome - the "is initial empty document" used for the Window reuse decision is set to false after the new about:blank committed, so future navigations won't reuse the Window. However the "is initial empty document" is still true for history-related decisions (and other Chrome decisions in the browser process), so history replacement will still happen. So we kinda have two "is initial empty document" states that don't agree with each other for the new about:blank case :(

However, from the test that @domenic linked, it looks like Firefox (the only browser passing the window sharing tests with the src='about:blank' case) agrees more with the "is initial empty document for history" state in Chrome (so, it still treats the new about:blank as the initial empty document for Window reuse), which is interesting.

Anyways, although I agree that the window reuse behavior is pretty weird and not interoperable, I guess we need to make sure it's not impossible to spec later after #6869. I wonder if splitting the value used for window reuse decision vs history replacement decision in the spec too makes sense?

@domenic
Copy link
Member

domenic commented Aug 3, 2021

I think this is already covered by this test, let me know if I need to add more!

I think that test will pass whether the iframe load event is sync or async. Could we expand it to test that the event is not fired sync?

@domenic
Copy link
Member

domenic commented Aug 3, 2021

Anyways, although I agree that the window reuse behavior is pretty weird and not interoperable, I guess we need to make sure it's not impossible to spec later after #6869. I wonder if splitting the value used for window reuse decision vs history replacement decision in the spec too makes sense?

Yeah, I am pretty convinced it will still be possible to spec after #6869. And indeed, one path would be splitting the "should reuse Window" boolean from the "is initial about:blank" boolean.

(In general I am pulled on two directions on Window object reuse. On the one hand, it's super-weird and annoying, so if we can minimize the number of cases it is visible to web developers, like Chrome and Safari seem to do, that seems good. On the other hand, unifying Window object reuse and history replacement like Firefox and the current spec does seems simpler, and simplicity is nice.)

@rakina
Copy link
Member Author

rakina commented Aug 5, 2021

I think that test will pass whether the iframe load event is sync or async. Could we expand it to test that the event is not fired sync?

Oh right! Updated, looks like only firefox fires it asynchronously, Chrome and Safari fires it synchronously :o (I guess because the commit is done synchronously as well)

Yeah, I am pretty convinced it will still be possible to spec after #6869. And indeed, one path would be splitting the "should reuse Window" boolean from the "is initial about:blank" boolean.

(In general I am pulled on two directions on Window object reuse. On the one hand, it's super-weird and annoying, so if we can minimize the number of cases it is visible to web developers, like Chrome and Safari seem to do, that seems good. On the other hand, unifying Window object reuse and history replacement like Firefox and the current spec does seems simpler, and simplicity is nice.)

Thanks, sounds good then. Hope we can end up with something sensible there!

@domenic
Copy link
Member

domenic commented Aug 5, 2021

Alright, I've updated #6863 to reflect the Safari/Chrome behavior for the iframe load event too. (And, it turns out, my reading of the spec was incorrect; before my latest push no load event would get fired for the initial insertion, for subtle reasons which are now explained in a note.)

I think that's probably the right move, since in general we're trying to align on 2/3 behavior here by explicitly acknowledging the initial insertion case as special.

So what remains is more re-reviews of #6869, and I believe we'd want to flip the test expectations to expect the event to be sync, instead of expecting it to be async.

domenic added a commit that referenced this issue Aug 10, 2021
For iframes: previously, we did a confusing thing where we would navigate to a non-initial about:blank. 2/3 engines instead just fire a synchronous load event at the iframe element, with no navigation. This is a bit simpler, and matches the popup case a bit better (after the below modifications).

For popups: previously, we fired the load event (but not the pageshow event) when the popup stays on the initial about:blank. 2/3 engines instead fire no events in this case, and the remaining one only fires it in the explicit "about:blank" case but not in the empty string case.

In both cases, it wasn't quite clear what to do when navigating to something like about:blank#foo or about:blank?foo. The spec now makes it clear that such cases cause URL updates of the new browsing context but not full navigations. In particular, for now at least the the initial about:blank-ness of the Document is retained. (In browsers, it seems like it's always retained for replacement vs. push purposes, but is only sometimes retained for Window object reuse purposes. #3267 tracks sorting that out.)

This also refactors the window open steps so that they have two primary branches, depending on whether to create a new window or not. Previously the steps were sorta unified, but there were a lot of consultations of the "new" variable throughout, which made it hard to understand the differences between the cases.

Closes #6863.
blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this issue Nov 1, 2021
Currently knowing whether a frame is showing the initial empty document
or not is hard to do because there are multiple places that try to
track the "initial empty document" state but actually fail to do so
due to various reasons:
- is_on_initial_empty_document_or_subsequent_empty_documents_ in
  FrameTreeNode and empty_document_status_ in FrameLoader are still
  true after multiple about:blank (and other about: URLs) commits that
  are not the initial/synchronous about:blank.
- has_committed_real_load_ in FrameTreeNode is used to determine the
  initial empty document state (it is used as input to
  empty_document_status_ in FrameLoader and IsInitialEmptyDocument() in
  Document), even though it does not consider document.open()

This CL fixes those problems and more:
- Sets is_on_initial_empty_document_or_subsequent_empty_documents_ and
  empty_document_status_ to false on the first commit that is not the
  initial/synchronous about:blank commit (and renames the former to
  is_on_initial_empty_document now that it actually represents the
  exact state)
- Removes uses of has_committed_real_load_ that use it to determine
  the initial empty document state (it's going to be removed completely
  in crrev.com/c/3244814)
- Renames and adds comments to various bits for easier understanding

After this CL, the "initial empty document" state tracking matches the
spec except that Chrome treats the initial about:blank document and the
synchronously loaded about:blank document as the same document (the
initial empty document), while the synchronously loaded about:blank
document does not exist in the spec (especially after
whatwg/html#6863).

Some subtle changes:
- about:blank documents that committed immediately after the
  initial/synchronous about:blank document are no longer treated as the
  initial empty document. This mainly affects history replacement
  decisions (since the initial empty document's history entry always
  gets replaced on the next navigation), so these documents' history
  entry will no longer be replaced based on their
  initial-empty-document-ness.
  Example of previous behavior:
  1. <iframe src="about:blank">. This will commit the synchronous
     about:blank document, and is correctly treated as the initial
     empty document.
  2. Navigate to about:blank or any other variants of it, e.g.
     about:blank?foo. This is a new document that is not the initial
     about:blank or the synchronous about:blank, but since it's an
     about:blank URL, it used to be treated as the initial empty
     document. With this CL, it's no longer treated as the initial
     empty document.
  3. Navigate to some other URL. This used to do replacement because
     we thought we're still on the initial empty document, but we
     actually are not. This CL fixes this case.
- Non-about:blank commits that accidentally go through the synchronous
  about:blank commit path are now correctly treated as non-initial
  empty document commits (but still go through the path for now, might
  be removed later). This includes all variants of about: URLs except
  for about:srcdoc that are not about:blank (e.g. about:mumble) - see
  the conditions in RenderFrameImpl::BeginNavigation(). This makes our
  behavior closer to the spec, which only allows about:blank
  variations to be treated as the initial empty document.

Bug: 1215096
Change-Id: Ic52bc562a4fbfdd92a63d980c75dd952a4e099d6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3238476
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#936787}
mfreed7 pushed a commit to mfreed7/html that referenced this issue Jun 3, 2022
For iframes: previously, we did a confusing thing where we would navigate to a non-initial about:blank. 2/3 engines instead just fire a synchronous load event at the iframe element, with no navigation. This is a bit simpler, and matches the popup case a bit better (after the below modifications).

For popups: previously, we fired the load event (but not the pageshow event) when the popup stays on the initial about:blank. 2/3 engines instead fire no events in this case, and the remaining one only fires it in the explicit "about:blank" case but not in the empty string case.

In both cases, it wasn't quite clear what to do when navigating to something like about:blank#foo or about:blank?foo. The spec now makes it clear that such cases cause URL updates of the new browsing context but not full navigations. In particular, for now at least the the initial about:blank-ness of the Document is retained. (In browsers, it seems like it's always retained for replacement vs. push purposes, but is only sometimes retained for Window object reuse purposes. whatwg#3267 tracks sorting that out.)

This also refactors the window open steps so that they have two primary branches, depending on whether to create a new window or not. Previously the steps were sorta unified, but there were a lot of consultations of the "new" variable throughout, which made it hard to understand the differences between the cases.

Closes whatwg#6863.
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
Currently knowing whether a frame is showing the initial empty document
or not is hard to do because there are multiple places that try to
track the "initial empty document" state but actually fail to do so
due to various reasons:
- is_on_initial_empty_document_or_subsequent_empty_documents_ in
  FrameTreeNode and empty_document_status_ in FrameLoader are still
  true after multiple about:blank (and other about: URLs) commits that
  are not the initial/synchronous about:blank.
- has_committed_real_load_ in FrameTreeNode is used to determine the
  initial empty document state (it is used as input to
  empty_document_status_ in FrameLoader and IsInitialEmptyDocument() in
  Document), even though it does not consider document.open()

This CL fixes those problems and more:
- Sets is_on_initial_empty_document_or_subsequent_empty_documents_ and
  empty_document_status_ to false on the first commit that is not the
  initial/synchronous about:blank commit (and renames the former to
  is_on_initial_empty_document now that it actually represents the
  exact state)
- Removes uses of has_committed_real_load_ that use it to determine
  the initial empty document state (it's going to be removed completely
  in crrev.com/c/3244814)
- Renames and adds comments to various bits for easier understanding

After this CL, the "initial empty document" state tracking matches the
spec except that Chrome treats the initial about:blank document and the
synchronously loaded about:blank document as the same document (the
initial empty document), while the synchronously loaded about:blank
document does not exist in the spec (especially after
whatwg/html#6863).

Some subtle changes:
- about:blank documents that committed immediately after the
  initial/synchronous about:blank document are no longer treated as the
  initial empty document. This mainly affects history replacement
  decisions (since the initial empty document's history entry always
  gets replaced on the next navigation), so these documents' history
  entry will no longer be replaced based on their
  initial-empty-document-ness.
  Example of previous behavior:
  1. <iframe src="about:blank">. This will commit the synchronous
     about:blank document, and is correctly treated as the initial
     empty document.
  2. Navigate to about:blank or any other variants of it, e.g.
     about:blank?foo. This is a new document that is not the initial
     about:blank or the synchronous about:blank, but since it's an
     about:blank URL, it used to be treated as the initial empty
     document. With this CL, it's no longer treated as the initial
     empty document.
  3. Navigate to some other URL. This used to do replacement because
     we thought we're still on the initial empty document, but we
     actually are not. This CL fixes this case.
- Non-about:blank commits that accidentally go through the synchronous
  about:blank commit path are now correctly treated as non-initial
  empty document commits (but still go through the path for now, might
  be removed later). This includes all variants of about: URLs except
  for about:srcdoc that are not about:blank (e.g. about:mumble) - see
  the conditions in RenderFrameImpl::BeginNavigation(). This makes our
  behavior closer to the spec, which only allows about:blank
  variations to be treated as the initial empty document.

Bug: 1215096
Change-Id: Ic52bc562a4fbfdd92a63d980c75dd952a4e099d6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3238476
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#936787}
NOKEYCHECK=True
GitOrigin-RevId: 86c88faacbba594af79e9e90af6eac0e451c6c20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

5 participants