Closed Bug 543435 (sync-about-blank) Opened 15 years ago Closed 5 days ago

Make initial about:blank loading into iframe not get overwritten by an async channel load

Categories

(Core :: DOM: Navigation, defect, P2)

Product:
Core
Component:
DOM: Navigation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
147 Branch
Project Flags:
Webcompat Priority P2
Tracking Flags:
Tracking Status
firefox147 --- fixed

People

(Reporter: hsivonen, Assigned: vhilla)

References

(Depends on 2 open bugs, Blocks 24 open bugs, Regressed 15 open bugs,
URL
)

Details

(Keywords: html5, webcompat:platform-bug)

User Story

platform-scheduled:2025-09-30
user-impact-score:460

Attachments

(3 files, 15 obsolete files)

Bug 543435 - Make initial about:blank not get overwritten by an async about:blank load.
48 bytes, text/x-phabricator-request
Details | Review
Bug 543435 - Ensure the initial about:blank SH entry is replaced. r=smaug
48 bytes, text/x-phabricator-request
Details | Review
Bug 543435 - Only reuse inner window for uncommitted initial documents. r=smaug!,asuth!
48 bytes, text/x-phabricator-request
Details | Review
Deferred from bug 533381 to move this away from the critical path of HTML5 parser default enablement on the trunk. Steps to reproduce: 1) Load http://hsivonen.iki.fi/test/moz/about-blank-load.html Actual results: Gecko generates an initial about:blank document into the iframe and then loads another about:blank document into the iframe asynchronously. Expected results: Opera loads about:blank synchronously once and fires an async load event. WebKit and IE8 load the about:blank synchronously and fire a sync load event. HTML5 (currently) says to generate the about:blank DOM synchronously and not to fire a load event at all. Since all browsers fire a load event currently, not firing it at all scares me. However, firing sync events while the parser is on the call stack isn't nice, either, so I suggest doing what Opera does. Attaching a patch that makes many Mochitests fail. I expect this patch to need tweaking in terms of the initial history item in the iframe. In any case, changing anything here is likely to require changes to existing test cases.
Component: DOM → Document Navigation
QA Contact: general → docshell
Blocks: 598895
Blocks: 563890
bz, is there an important performance reason why the creation of the initial about:blank is created lazily? If not, I'd like to make new content viewer creation create the initial about:blank eagerly.
I don't know, offhand. I think nowadays we always end up creating it, basically, so I doubt there is one.
Does this look like something that's worthy of starting the test case adjustment effort? Note that this doesn't handle non-initial navigation to about:blank in a special way. I haven't yet done the research to see what's needed in that case.
Assignee: nobody → hsivonen
Attachment #424566 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #527253 - Flags: review?(bzbarsky)
The right patch this time.
Attachment #527253 - Attachment is obsolete: true
Attachment #527268 - Flags: review?(bzbarsky)
Attachment #527253 - Flags: review?(bzbarsky)
Attachment #527268 - Attachment is obsolete: true
Attachment #527268 - Flags: review?(bzbarsky)
Does this look like an acceptable approach? This patch yields the results I was after for the purposes of Web content and doesn't break the browser UI (at least not in obvious ways).
Attachment #529463 - Flags: review?(bzbarsky)
Attachment #529463 - Attachment is obsolete: true
Attachment #529463 - Flags: review?(bzbarsky)
Quoting bug 372964 comment #31: > IsInitialDocument is the first document ever loaded in the window, and in > particular one whose inner window might need to be persisted across document > loads. This used to be about:blank in some cases... but with your changes we > might have more than one initial document, depending on how they are done. :( As far as I can tell, about:blank needs to be the initial document until it fires its load event at which point it becomes no longer initial in order to get the same effect as a parsed about:blank loading on top of a generated about:blank previously.
Depends on: 660200
So I'm trying to get the load event fired by calling loadGroup->RemoveRequest(mChannel, nsnull, NS_OK); from a runnable. This makes the window.open()ed window in dom/tests/mochitest/general/test_bug631440.html never fire its load event. (That is, putting printfs in the code that's supposed to dispatch the event doesn't run. ) I have trouble figuring out why the code doesn't work in this case. Should I try firing the load event for the generated about blank completely manually (creating the event object and firing it)?
(In reply to comment #8) > I have trouble figuring out why the code doesn't work in this case. loadGroup->RemoveRequest ends up firing the load event for a different docshell/viewer/doc combination. bz, any ideas how this could happen? I'm guessing another document somehow ends up in the same load group, but I can't tell where that could happen.
(In reply to comment #9) > (In reply to comment #8) > > I have trouble figuring out why the code doesn't work in this case. > > loadGroup->RemoveRequest ends up firing the load event for a different > docshell/viewer/doc combination. Turns out it end up firing the load event for the opener but not for the openee.
Progress! I needed to reset mEODForCurrentDocument at the right moment.
Attachment #529720 - Attachment is obsolete: true
Still chrome mochitests failing...
Attachment #536073 - Attachment is obsolete: true
With proper principal in some cases.
Attachment #541670 - Attachment is obsolete: true
Blocks: 667227
Depends on: 668209
http://hg.mozilla.org/try/rev/57ec25cc3a3d 10.204 + nsCOMPtr<nsIInterfaceRequestor> requestor = do_QueryInterface(mShell); 10.205 + 10.206 + nsCOMPtr<nsILoadGroup> loadGroup; 10.207 + 10.208 + nsresult rv = requestor->GetInterface(NS_GET_IID(nsILoadGroup), 10.209 + getter_AddRefs(loadGroup)); could be written nsCOMPtr<nsILoadGroup> loadGroup = do_GetInterface(mShell, &rv);
(In reply to comment #14) > could be written > > nsCOMPtr<nsILoadGroup> loadGroup = do_GetInterface(mShell, &rv); Thanks. I've made that change. (Aside: Is there any reason other than "legacy" for the interface requestor pattern exist in the code base now that we are no longer pretending that interfaces are frozen forever and we aren't expecting random third parties to swap out implementations from behind contract ids and interfaces?)
Attachment #542132 - Attachment is obsolete: true
> and we aren't expecting random third parties to swap out implementations For some contracts, we still are.
Depends on: 684690
Depends on: 681392
Attachment #545945 - Attachment is obsolete: true
Blocks: 695868
Depends on: 744366
Depends on: 775467
Depends on: 779100
Still orange but works for dog food.
Attachment #562404 - Attachment is obsolete: true
Depends on: 779430
Depends on: 779959
No longer depends on: 779430
Blocks: 473396
Assignee: hsivonen → nobody
Status: ASSIGNED → NEW
See Also: → 1578379
Depends on: 1634653
Blocks: 1634653
No longer depends on: 1634653
Blocks: 1745806

Please don't get excited about the patch at this point. It's a dirty hack, and we don't yet know how the test suite tolerates it.

Note that the patch not only changes how channel loads of the initial about:blank work but also changes later about:blanks to go through the normal parsing path.

Let's see how orange everything goes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9e3d6f45aed9cbc21178d092069ce25e0e7bc45

(In reply to Henri Sivonen (:hsivonen) from comment #27)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c171e7501d3789374b89ccac1d023dfaa5a9d1a5

It's rather encouraging how little in WPT breaks even if Gecko-specific tests are very orange.

(In reply to Henri Sivonen (:hsivonen) from comment #26)

but also changes later about:blanks to go through the normal parsing path.

The latest patch rolls back this change. Also, the try run includes the patch from bug 1736570 to turn off DocumentChannel for the initial about:blank.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=982764965be9e1af0a9869209ed6321cbb234b29

OK, now it's slightly encouraging that there are lots of failures where the log line implicates about:blank directly, which suggests it isn't now an "everything is broken" situation.

Blocks: 1736717
See Also: → 1745638
No longer depends on: 1724975
Webcompat Priority: --- → ?
Attachment #648310 - Attachment is obsolete: true
Attachment #9256504 - Attachment is obsolete: true

FWIW, things go very orange if calling the lazy EnsureContentViewer eagerly upon docshell initialization:
https://treeherder.mozilla.org/jobs?repo=try&revision=dec017d45932941e5b088dd398c3e0404b0e3434

It's not a great sign that we implicitly depend on the lazy creation running somewhat later.

That doesn't look very orange to me :)
Many of the failures are there more than once and usually an orange in a test group seems to be about just one or two test failing.
I'd assume debugging and fixing some failure might fix also many other failures. (crossing fingers)

Webcompat Priority: ? → P2
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED

Looks like I broke window.open().

Note to self: Revisit the query string versions of
./mach wpt /html/browsers/browsing-the-web/navigating-across-documents/initial-empty-document/initial-content-replacement.html

(In reply to Henri Sivonen (:hsivonen) from comment #49)

Note to self: Revisit the query string versions of
./mach wpt /html/browsers/browsing-the-web/navigating-across-documents/initial-empty-document/initial-content-replacement.html

Also ./testing/web-platform/tests/html/browsers/browsing-the-web/navigating-across-documents/initial-empty-document/load-event-iframe-element.html

(In reply to Henri Sivonen (:hsivonen) from comment #50)

(In reply to Henri Sivonen (:hsivonen) from comment #49)

Note to self: Revisit the query string versions of
./mach wpt /html/browsers/browsing-the-web/navigating-across-documents/initial-empty-document/initial-content-replacement.html

Also ./testing/web-platform/tests/html/browsers/browsing-the-web/navigating-across-documents/initial-empty-document/load-event-iframe-element.html

nsDocShell::UpdateURLAndHistory

Let's see if this disconnects too many event targets upon inner window reuse:
https://treeherder.mozilla.org/jobs?repo=try&revision=57c25730a11089bfa87f4f6edb65b147434f074e

Primarily a rebase (also removed some failure expectations):
https://treeherder.mozilla.org/jobs?repo=try&revision=e8b94c3f9bee564acef045780af3a87ca7293cfa

The failures in test_bug346659.html might not be bugs. It would be worthwhile to port the non-SpecialPowers parts of that test to WPT to see what other browsers do.

In the case of toolkit/components/extensions/test/mochitest/test_ext_storage_cleanup.html , the parent process tries to get the message manager from the outer window for moz-extension://43973589-61d5-49d1-8144-18b51a210399/_generated_background_page.html and the message manager is null.

Performance baseline for comparison with the try run in the previous comment:
https://treeherder.mozilla.org/jobs?repo=try&revision=8770e66af1ae220309b46a30608653ac27b8f2eb

See Also: → 1792685

kmag, what should I conclude from https://searchfox.org/mozilla-central/source/js/xpconnect/tests/unit/test_nuke_webextension_wrappers.js still returning bar instead of something matching "dead object"?

AFAICT, docShell.createAboutBlankContentViewer(principal, principal); replaces the initial about:blank with another synthetic about:blank with custom principal.

Flags: needinfo?(kmaglione+bmo)
Severity: normal → S3

The severity field for this bug is relatively low, S3. However, the bug has 5 duplicates and 11 votes.
:hsivonen, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Flags: needinfo?(hsivonen)

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.

Flags: needinfo?(hsivonen)

Try run with built-in WebExtensions hopefully disabled for real this time:
https://treeherder.mozilla.org/jobs?repo=try&revision=4f2ba9c8c1178a8477046f1d1f689d3623823a35

accessible/tests/mochitest/treeupdate/test_doc.html runs into principal inheritance changes. The patch makes the system principal inherit into about:blank in some system contexts. One option would be to try what would happen if we never inherited the system principal in to about:blank. If it needs to inherit in some conditions but not others, it would be important to be able to articulate a rule in a better way than "whatever happened with the previous more ad hoc code".

M(4, 5) Failures with _ext_ in the test name are about bug 1792685, AFAICT.

M(6) Various scrolling-related failures seem to be pre-existing brittleness in the scrolling test harness. At least spot checking suggests that there problems that look like timing issues instead of having clearer reasons.

Major rework of how onload fires:
https://treeherder.mozilla.org/jobs?repo=try&revision=1b2f299e99e15a44d92f3c98da38c26b5e86fcba
This runs much more onload-adjacent code than the patch did previously. This should address problems with windows opened with window.open("about:blank") never getting focus.

testing/web-platform/tests/service-workers/service-worker/resources/nested-iframe-parent.html change is a hack that really needs a follow-up.

Depends on: 1800973
Blocks: 207842

At least one of the about:blank ServiceWorker WPTs should go green:
https://treeherder.mozilla.org/jobs?repo=try&revision=2f75aa80c9592adf14c40d943ace1f3aea18891d

/html/anonymous-iframe/local-storage-initial-empty-document.tentative.https.window.html seems to fail without the patch, too.

Failures in /html/browsers/history/the-history-interface/004.html are known intermittents and the test also intermittently passes with the patch.

browser_windowPrompt.js passes if I use data:text/html, in place of about:blank in the argument for addTab. It's possible that this isn't about synchronicity but about whether the inner window is reused for whatever is loaded next. (For u2f tests, it seems that the issue was reusing an inner window created before the u2f-enabled pref had taken effect.)

In the case of accessible/tests/browser/e10s/browser_caching_document_props.js, it appears that the accessibility code sees the CanonicalBrowsingContext's creation-time URL instead of seeing the situation after receiving an update from a content process.

browser_windowPrompt.js

Oops, wrong test. I meant toolkit/components/cookiebanners/test/browser/browser_cookieinjector.js

test_ext_webrequest_filter.html superficially looks like another case of failure to inherit the ServiceWorker controller across window.open.

Try to inherit the controller for ServiceWorkers from the opener:
https://treeherder.mozilla.org/jobs?repo=try&revision=cd3e8619ff3d77c2eb1e6e441145914cc387930b

Regressions: 1854601

(In reply to Henri Sivonen (:hsivonen) from comment #69)

kmag, what should I conclude from https://searchfox.org/mozilla-central/source/js/xpconnect/tests/unit/test_nuke_webextension_wrappers.js still returning bar instead of something matching "dead object"?

AFAICT, docShell.createAboutBlankContentViewer(principal, principal); replaces the initial about:blank with another synthetic about:blank with custom principal.

Findings so far:

Without the patch, the real initial about:blank is lazy and hasn't actually been created by the time of the call to docShell.createAboutBlankContentViewer(principal, principal);. Therefore, the inner window resulting from that call is the only inner window that has occupied the outer window.

With the patch, by the time docShell.createAboutBlankContentViewer(principal, principal); is called, the inner window for the initial about:blank has already been created eagerly and this call dooms it.

I think the test jumps the gun by awaiting for the nuking of first inner window. However, trying to await for the nuking of the right inner window (either by awaiting twice or by filtering on the inner window id) times out.

I don't understand why the test can't observe the notification of the nuking of the right inner window.

Comment out waiting for browser-delayed-startup-finished in test harness:
https://treeherder.mozilla.org/jobs?repo=try&revision=f82ef82ca310def0e6bb54200392be9aea2a1bc5

(In reply to Henri Sivonen (:hsivonen) from comment #122)

Comment out waiting for browser-delayed-startup-finished in test harness:

This appears to have been a bad idea. New run with waiting for browser-delayed-startup-finished restored:
https://treeherder.mozilla.org/jobs?repo=try&revision=1a1afc289ee597809b6dfbb9035b2031d80958fd

Depends on: 1856329
Depends on: 1856330
Priority: -- → P2

toolkit/components/antitracking/test/browser/browser_fpiServiceWorkers_fingerprinting.js and wpt /service-workers/service-worker/about-blank-replacement.https.html have conflicting requirements for whether the effective storage principal for the initial about:blank should be the node principal or the partitioned principal.

Depends on: 1829594

Revisit dom/base/test/test_bug1730284.html after bug 1829594.

Depends on: 1857455

Notable change to getting the UI react correctly when about:blank is the document that a newly-opened content window stays at:
https://treeherder.mozilla.org/jobs?repo=try&revision=4096e62d56883a0329ffc71ca6108e01289f9e98

Adding a script blocker around docshell initialization didn't work, because top-level SetCurrentURI asserts that there isn't a blocker:
https://treeherder.mozilla.org/jobs?repo=try&revision=798f73af9039d2717f2bc17ebacc23d00916cca6

Moving the offending notification to later without a script blocker:
https://treeherder.mozilla.org/jobs?repo=try&revision=e1a78954c45e69eef3324c5a56d3a652e3110529

Consistently not firing onLocationChange at initial about:blank creation time (even top-level) and only firing it for the initial about:blank if that ends up being the actual first navigation destination:
https://treeherder.mozilla.org/jobs?repo=try&revision=a63276942a16b7daf728bceb98108997129d358b

Check for contentness of HTML iframes in the parent process for the purpose of principal inheritance:
https://treeherder.mozilla.org/jobs?repo=try&revision=64c3f2a714c6be7091bd711295cb6e1f3f39ee60

(In reply to Henri Sivonen (:hsivonen) from comment #139)

Check for contentness of HTML iframes in the parent process for the purpose of principal inheritance:
https://treeherder.mozilla.org/jobs?repo=try&revision=64c3f2a714c6be7091bd711295cb6e1f3f39ee60

And inherit the origin attributes from the BrowsingContext when not inheriting the principal:
https://treeherder.mozilla.org/jobs?repo=try&revision=614dc5b81fbec76ff51bed7a28dc9107208999a3

Depends on: 1859484
Depends on: 1859490

Introduce an isUncommittedInitialDocument flag that is true if isInitialDocument and the browsing context hasn't started navigation to about:blank. This enables various filtering to not filter out initial documents that end up remaining in the browsing context.
https://treeherder.mozilla.org/jobs?repo=try&revision=c8524aa8459ba0f6abdcc7f45638086d10e1cfdc

The isInitialDocument changes were probably too much for the "remote" test set:
https://treeherder.mozilla.org/jobs?repo=try&revision=f3d866993d5dca50aca476cccd08ed15d7598b68

Depends on: 1862935
Attachment #9291117 - Attachment description: Bug 543435 - Make initial about:blank not get overwritten by an async about:blank load. → WIP: Bug 543435 - Make initial about:blank not get overwritten by an async about:blank load.

Redirect a needinfo that is pending on an inactive user to the triage owner.
:farre, since the bug has recent activity, could you have a look please?

For more information, please visit BugBot documentation.

Flags: needinfo?(kmaglione+bmo) → needinfo?(afarre)

Rebase and remove a couple of redundant-looking createAboutBlankContentViewer() calls:
https://treeherder.mozilla.org/jobs?repo=try&revision=fe10bb9e4addde99f9c8c4daceae72e3862d7466

Try to prevent treatment of an about:blank load as an initial about:blank load in some cases, especially when continuing a cross-process redirect:
https://treeherder.mozilla.org/jobs?repo=try&revision=2379c26fa772262689de9691166b52efc994e8bb

Regressions: 1865028

This move the initial client source creation to between connecting the document viewer and firing the location change events:
https://treeherder.mozilla.org/jobs?repo=try&revision=f8271fca8f9e9ab0845ac34eef466549aae0a03f

Omit the initial viewer for new tabs that don't have an opener.
https://treeherder.mozilla.org/jobs?repo=try&revision=8709dc18c2636bcca68145f4c7435de5945e6bad

Rebase (the base patch that doesn't try to omit the initial viewer):
https://treeherder.mozilla.org/jobs?repo=try&revision=8b511df083a851f11c3829ee869adf090f49644e

Rebase and also revert towards the old behavior for top-level content browsing contexts that don't have an opener in order to (hopefully) break fewer frontend tests.

https://treeherder.mozilla.org/jobs?repo=try&revision=00d5351e4d988ec660edcf71f77a2f099213ccd3

Blocks: 1875059

Try to make upgrade insecure requests on the initial about:blank work:
https://treeherder.mozilla.org/jobs?repo=try&revision=5b19acb308c73fa6dd8ba7df9f7b7ea7b5baa707

Blocks: 1890053
Blocks: 1573262

Verified this issue and still reproduces on Firefox Nightly. Accessing: https://hsivonen.com/test/moz/about-blank-load.html returns different results when viewing the returned text order (for example, History length: 2 is the last entry in Firefox)

Tested with:

Browser / Version: Firefox Nightly 128.0a1 (2024-05-21) (64-bit)
Operating System: Windows 10 PRO x64

Blocks: 1899523
Blocks: 1362086
No longer blocks: 1362086
Depends on: 1362086
Blocks: 1362086
No longer depends on: 1362086

Here is a summary of the wpt failures I looked at so far. I'll highlight the more interesting / still relevant parts.

eval-blocked-in-about-blank-iframe

A call to Document::InitCSP is missing for an about:blank document.

  • We write code to an about:blank iframe. This code calls eval and listens for the securitypolicyviolation event. If the event is received, a message is sent to the parent document, which is awaiting this message.
  • But the event is incorrectly fired on the parent document, instead of iframe.
  • In Gecko, eval leads to nsCSPContext::FireViolationEvent, where the event target is set to mLoadingContext.
    • With this patch, this context stems from InitFromOther here in nsDocShell::CreateAboutBlankDocumentViewer, which is reached from nsDocShell::InternalLoad. The iframe inherits the parent documents CSP and the request context is set to the parent document.
    • Without this patch, InitFromOther is called in the same way. But the async channel load calls into Document::InitCSP via Document::StartDocumentLoad, which sets the request context to the iframe document.

We could invoke cspToInherit->SetRequestContextWithDocument(blankDoc) after InitFromOther in CreateAboutBlankDocumentViewer. This fixes the test, but I'm unsure if other initialization tasks from StartDocumentLoad are also missing.

declarative-shadow-dom-write-to-iframe

DSD was implemented in Bug 1712140 and put behind a pref in D194846. This pref is used in nsContentDLF::CreateDocument to initialize doc->SetAllowDeclarativeShadowRoots. For about:blank documents in nsContentDLF::CreateBlankDocument, this does not happen and the parser ignores DSD. We can probably just add the same call to CreateBlankDocument.

iframe-loading-lazy-nav-link-click-fragment

  • We have an <iframe src=... loading=lazy hidden> and an <a target=iframe href=about:blank#fragment>. We click the link, then show the iframe.
  • Clicking the link calls into InternalLoad via PerformRetargeting and nsDocShell::OnLinkClickSync.
    • Without the patch IsSameDocumentNavigation returns false as mCurrentURI is null. We reachDoURILoad where we CancelLazyLoading and perform an async about:blank load.
    • With the patch, mCurrentURI is about:blank and InternalLoad early-returns with HandleSameDocumentNavigation. No load event is fired, lazy loading is not canceled. When the iframe is shown, we lazy load src. mCurrentURI is set due to the patch from EnsureDocumentViewer in nsDocShell::Initialize.

Considering the spec, I think we should cancel lazy loading already in InternalLoad before HandleSameDocumentNavigation. To navigate a navigable, we cancel lazy loading in step 10 before 'navigate a fragment' in step 13. This makes the test time out as we never fire an about:blank load event, same as in Blink.

I'm unsure what the spec says on the interaction of lazy loading and about:blank. For once, when clicking the link and navigating the iframe, what should the 'active session history entry' be? Gecko and Blink behave as if it's 'about:blank' then 'navigate a fragment' and never fire a load event. If the iframe doesn't have a 'src' attribute but remains lazy and hidden, Blink fires a sync 'about:blank' load on binding, Gecko still never fires a load event.

(In reply to Vincent Hilla [:vhilla] from comment #180)

We could invoke cspToInherit->SetRequestContextWithDocument(blankDoc) after InitFromOther in CreateAboutBlankDocumentViewer. This fixes the test,

It makes sense to do this.

but I'm unsure if other initialization tasks from StartDocumentLoad are also missing.

tschuster, can you give advice about this?

DSD was implemented in Bug 1712140 and put behind a pref in D194846. This pref is used in nsContentDLF::CreateDocument to initialize doc->SetAllowDeclarativeShadowRoots. For about:blank documents in nsContentDLF::CreateBlankDocument, this does not happen and the parser ignores DSD. We can probably just add the same call to CreateBlankDocument.

Makes sense.

I'm unsure what the spec says on the interaction of lazy loading and about:blank.

It probably makes sense to do the closest to what Blink does and then to file a spec bug.

Flags: needinfo?(tschuster)

Document::InitCSP does all kinds of initialization besides SetRequestContextWithDocument, I don't see how we could skip it. Some things like handling the HTTP Content-Security-Policy header presumably don't apply. Other stuff like the call to Document::ApplySettingsFromCSP seems quite necessary. Not sure about handling of the sandbox flag.

Flags: needinfo?(tschuster)

Two more tests

iframe-blank-inherit.meta/upgrade/iframe-tag

I didn't get into the details of what the test does, as it uses several utility functions making it difficult to isolate a single, short testcase.

The test is for the upgrade-insecure-requests CSP. With the about:blank patch, we no longer call loadInfo->SetUpgradeInsecureRequests nor Document::ApplySettingsFromCSP via InitCSP, as :tschuster noted. Before ApplySettingsFromCSP would be called, there is also mUpgradeInsecureRequests = loadInfo->GetUpgradeInsecureRequests() in StartDocumentLoad.

I see that this patch introduces ApplyCspFromLoadInfo, which does mUpgradeInsecureRequests = loadInfo->GetUpgradeInsecureRequests() for about:blank where StartDocumentLoad doesn't happen. But that loadInfo is created a few lines before without ever invoking SetUpgradeInsecureRequests. So it doesn't help. I tried replicating loadInfo->SetUpgradeInsecureRequests from here in DoURILoad, but that didn't help either.

What would fix the test is doing doc->ApplySettingsFromCSP(false) in DoURILoad within the isAboutBlankLoadOntoInitialAboutBlank branch. I'm concerned that I'm cherry picking lines from StartDocumentLoad till tests work, but other things might still be missing.

document-base-url-window-initiator-is-not-opener

Edit: this test uses window.open() without specifying about:blank which appears to be broken. Fixing this first might change how this test behaves. I'm thinking that the navigation to a different base url shouldn't actually lead to the sync about:blank handling.

The problem is that when setting location.href=about:blank on an about:blank page, we don't fire pagehide nor change the baseURI. Secondary to fixing the test, there might be web interop problems here, as we fire a second sync load event, unlike Blink.

In the test, we have some document with baseURI A that opens an about:blank popup. It also contains an iframe with a different baseURI B. When the iframe is loaded, it initiates a navigation via location.href = about:blank on the popup. The test expects that the popup changes to baseURI B and fires a pagehide. The test allows for this to happen async and doesn't consider whether there should be a second load event. Blink changes baseURI sync, fires pagehide async, then changes baseURI and fires no load (or clears the load listener beforehand?).

We can also consider two similar cases: the iframe has the same baseURI as the parent or the parent itself does location.href=about:blank. In both cases, Blink will fire pagehide and change the baseURI from about:blank to the parent document's URI. For Gecko with the about:blank patch, nothing happens in the latter case while the former case only triggers a sync load event.

Regarding the cause for this test failure. Without this patch, the baseURI is set in Document::Reset, reached via Document::StartDocumentLoad from the async channel load. pagehide is fired from FirePageHideNotification during nsDocShell:CreateDocumentViewer, also caused by the channel load.

I tried to reconstruct what the spec says. Navigate a navigable step 22.5.2 sets 'about base URL' in parallel and fires the pagehide much later in unloading-documents via applying the history step. I understand this so that the baseURI should be changed async. I didn't find under which conditions navigate a navigable causes a load event.

I don't really know how to fix the test. Always firing pagehide for about:blank from DoURILoad causes the browser to become blank. Calling Document::Reset(aboutBlankChannel) causes a crash and wouldn't fix the test anyway, as baseURI was null here for the aboutBlankChannel. Both nsIURI and nsINestedURI don't support getting the baseURI so I wouldn't really know how to obtain it any other way without directly depending on nsNestedAboutURI.

Perhaps interesting, but less relevant are how browsers differ if we were to have a second iframe with baseURI C causing a second navigation of the popup. Blink will then block access to the popup due to cross-origin.

window.open() doesn't lead to nsDocShell::DoURILoad while window.open('about:blank') does and this causes multiple test failures. There was a merge conflict that I didn't know how to resolve and now I see why the conflicting code was needed.

Per the spec window-open-steps, we determine whether target leads to a new window in step 13 and consume user activation. In step 15.3, we then make an empty url correspond to about:blank iff target leads to a new window.

From my understanding, we consume user activation here in nsGlobalWindowOuter::OpenInternal due to Bug 1901139. Later in nsWindowWatcher::OpenWindowInternal we determine whether target leads to a new window as indiciated by windowIsNew. Per :edgars change in Bug 1901139, we want to create the load state before use activation is consumed. But we need the URI to create the load state and for an empty URL, we'll only have it later in OpenWindowInternal.

From the patch in nsWindowWatcher.cpp line 1317 in OpenWindowInternal

if (windowIsNew && !uriToLoad && aNavigate && !loadState) {
  NS_NewURI(getter_AddRefs(uriToLoad), "about:blank"_ns);
  // create loadState
}

and from Bug 1901139 in nsGlobalWindowOuter.cpp line 6808 in OpenInternal

RefPtr<nsDocShellLoadState> loadState = aLoadState;
  if (!loadState && aNavigate && uri) {
    loadState = nsWindowWatcher::CreateLoadState(uri, this);
  }

:edgar you authored Bug 1901139 and particularly, moved load state creation to nsGlobalWindowOuter here. Can you give advice?

Flags: needinfo?(echen)
Assignee: hsivonen → vhilla

I'm a bit stuck on browser_firstPartyIsolation_js_uri.js. This test loads javascript:0; but browser_firstPartyIsolation_aboutPages.js is also failing with about:blank.

:smaug do you have some idea what to do? With this patch, the firstPartyOrigin origin attribute is not set on about:blank pages. I guess we either have to change the document principal of the initial document, or set firstPartyDomain for the initial principal created during ConstructBrowser.

I looked at the javascript:0; case with pernosco.

  • Before this patch, loading this URI leads to RecvLoadURI in the content process. We create a new null principal in nsDocShellLoadState::SetupInheritingPrincipal and set firstPartyDomain on the origin attributes. During the following channel load, Document::Reset sets the document principal to this null principal.
  • With the patch, we still create the null principal during RecvLoadURI. But nsDocShell::DoURILoad early-returns due to the sync about:blank handling and the document principal doesn't get updated. When the test later checks the document origin, it finds the one created during RecvConstructBrowser, which doesn't have a firstPartyDomain set (it's empty).

I cannot call Document::Reset or Document::SetPrincipal in DoURILoad during the sync about:blank handling, as this fails AssertDocGroupMatchesKey.

Flags: needinfo?(smaug)

(In reply to Vincent Hilla [:vhilla] from comment #184)

:edgar you authored Bug 1901139 and particularly, moved load state creation to nsGlobalWindowOuter here. Can you give advice?

I responded to this in https://phabricator.services.mozilla.com/D155376#8106563.

Flags: needinfo?(echen)

(In reply to Vincent Hilla [:vhilla] from comment #185)

I'm a bit stuck on browser_firstPartyIsolation_js_uri.js. This test loads javascript:0; but browser_firstPartyIsolation_aboutPages.js is also failing with about:blank.

This seems related to browser_hwconcurrency_popups_aboutblank.js. The test is a bit lengthy, but I got a simplified version. In a page exempted from RFP, open an about:blank popup and eval(navigator.hardwareConcurrency). With this patch, it's incorrectly spoofed. From pernosco, Document::mShouldResistFingerprinting is incorrectly true as we don't call RecomputeResistFingerprinting during the sync about:blank load, or update the principal at all.

Edit: hwconcurrency fixed by doing Document::SetContainer slightly earlier.

(In reply to Vincent Hilla [:vhilla] from comment #185)

or set firstPartyDomain for the initial principal created during ConstructBrowser.

That would be better. Documents should have the right principal from the beginning if just possible.

I looked at the javascript:0; case with pernosco.

  • Before this patch, loading this URI leads to RecvLoadURI in the content process. We create a new null principal in nsDocShellLoadState::SetupInheritingPrincipal and set firstPartyDomain on the origin attributes. During the following channel load, Document::Reset sets the document principal to this null principal.
  • With the patch, we still create the null principal during RecvLoadURI. But nsDocShell::DoURILoad early-returns due to the sync about:blank handling and the document principal doesn't get updated. When the test later checks the document origin, it finds the one created during RecvConstructBrowser, which doesn't have a firstPartyDomain set.

Hmm, it would be best if we could get firstPartyDomain from the beginning.
I guess you mean that we don't get it as part of https://searchfox.org/mozilla-central/rev/a9824b3ffc2efea7a317f114bc3192ccdffa0ef7/dom/ipc/WindowGlobalTypes.ipdlh#20,26 ? Or is the principal created later?
(I'm still trying to figure out when first party should be true. )

Flags: needinfo?(smaug)
Flags: needinfo?(allstars.chh)

According to the spec, canceling lazy loading should happen before
navigating to a fragment. If we cancel lazy loading in DoURILoad,
we won't cancel for same document navigation.

To my knowledge, this hasn't been an issue. But with sync-about-blank,
this would break wpt iframe-loading-lazy-nav-link-click-fragment.html.

User Story: (updated)

(In reply to Olli Pettay [:smaug][bugs@pettay.fi] from comment #188)

I guess you mean that we don't get it as part of https://searchfox.org/mozilla-central/rev/a9824b3ffc2efea7a317f114bc3192ccdffa0ef7/dom/ipc/WindowGlobalTypes.ipdlh#20,26 ?

Right.

CreateBrowser sets aWindowInit.principal() to nsOpenWindowInfo::PrincipalToInheritForAboutBlank(), which is sent to the child and thereby cloned. For this principal, the firstPartyDomain is empty and const.

From RecvConstructBrowser, we call BrowserChild::Init, nsWebBrowser::Create, nsDocShell::InitWindow, Initialize, EnsureDocumentViewer, CreateAboutBlankDocumentViewer, nsContentDLF::CreateBlankDocument, nsHTMLDocument::ResetToURI, Document::ResetToURI, and finally Document::SetPrincipals.

From what I see, the firstPartyDomain is usually set up in nsDocShellLoadState::SetupInheritingPrincipal() and needs an URI, which we don't have during browser construction. But at the end of browser creation, the Document principal is set, so this is where we would have to create a new principal.

Forward the ni? to Tim for Vincent's questions in comment 185 and comment 187

Flags: needinfo?(allstars.chh) → needinfo?(tihuang)

So the question is that in which cases should firstPartyDomain be set when dealing with about:blank documents, and what the value should be?

We have a unique firstPartyDomain for the about URLs that aren't loaded with the system principal. The unique firstPartyDomain is used if the given URI is an about URL when setting the firstPartyDomain to OriginAttributes. So, I think we should set the firstPartyDomain as soon as we know the URL is an about:blank page that doesn't inherit any principal.

Flags: needinfo?(tihuang)
Depends on: 1945452
Depends on: 1945808

(In reply to Tim Huang[:timhuang] from comment #193)

We have a unique firstPartyDomain for the about URLs that aren't loaded with the system principal. The unique firstPartyDomain is used if the given URI is an about URL when setting the firstPartyDomain to OriginAttributes. So, I think we should set the firstPartyDomain as soon as we know the URL is an about:blank page that doesn't inherit any principal.

For the test browser_firstPartyIsolation_aboutPages.js test_remote_window_open_aboutBlank, the origin was previously e.g. moz-nullprincipal:{3f4920fe-8d1d-4e11-9032-c66ca5aa8bff}^firstPartyDomain=3f4920fe-8d1d-4e11-9032-c66ca5aa8bff.mozilla, which is different from ABOUT_URI_FIRST_PARTY_DOMAIN. We got this one from here. The test appears as if it doesn't expect to see ABOUT_URI_FIRST_PARTY_DOMAIN, as there is also test_aboutURL explicitly testing about pages, except for about:blank.

I think we want to do something like

nsCOMPtr<nsIURI> nullPrincipalURI = NullPrincipal::CreateURI(nullptr);
OriginAttributes attrs(aBrowsingContext->OriginAttributesRef());
attrs.SetFirstPartyDomain(true, nullPrincipalURI);
nsCOMPtr<nsIPrincipal> initialPrincipal = NullPrincipal::Create(attrs, nullPrincipalURI);

in CreateBrowser. But then, this assert fails. Though the tests pass.

But assuming I do want to use ABOUT_URI_FIRST_PARTY_DOMAIN. With the sync-about-blank patch, our principal comes from CreateBrowser here. If I want to apply ABOUT_URI_FIRST_PARTY_DOMAIN and not hit above assert, I can set the firstPartyDomain e.g. here. But the test will still fail, as mOriginNoSuffix and firstPartyDomain won't match. I.e. I then have an origin moz-nullprincipal:{b81259fc-60c2-4052-8ccb-d8f9748a242b}^firstPartyDomain=about.ef2a7dd5-93bc-417f-a698-142c3116864f.mozilla.

Blocks: 1944543
Depends on: 1948216
Depends on: 1949168
Depends on: 1478596
No longer depends on: 1478596
Depends on: 1950418
Blocks: 1627844
Depends on: 1955324
Blocks: 116023
Depends on: 1960620
Attachment #9460170 - Attachment description: WIP: Bug 543435 - Cancel lazy loading already in InternalLoad. r=#dom-core,sefeng → Bug 543435 - Cancel lazy loading already in InternalLoad. r=#dom-core,sefeng
Attachment #9291117 - Attachment description: WIP: Bug 543435 - Make initial about:blank not get overwritten by an async about:blank load. → Bug 543435 - Make initial about:blank not get overwritten by an async about:blank load.
Depends on: 1962976

Comment on attachment 9460170 [details]
Bug 543435 - Cancel lazy loading already in InternalLoad. r=#dom-core,sefeng

Revision D234665 was moved to bug 1962976. Setting attachment 9460170 [details] to obsolete.

Attachment #9460170 - Attachment is obsolete: true
Depends on: 1965437
User Story: (updated)
Depends on: 1967705
See Also: → 1971272
Blocks: 1971796

The _ConfigurationModule blocks the parser to perform some configuration tasks
on new documents. But parser blocking doesn't work anymore for the initial
about:blank document. This patch ensures that browsing_context.create doesn't
complete before configuration tasks are done.

See Also: → 1972865
Blocks: 1973218

Comment on attachment 9494402 [details]
Bug 543435 - Don't rely on parser blocking in webdriver for initial page load. r=jdescottes

Revision D253541 was moved to bug 1967705. Setting attachment 9494402 [details] to obsolete.

Attachment #9494402 - Attachment is obsolete: true
See Also: → 1974454
Duplicate of this bug: 1975184
No longer depends on: 1955324
Regressions: 1955324
See Also: → 1955324
No longer depends on: 1948216
Regressions: 1948216
See Also: → 1948216
Duplicate of this bug: 1950418
Duplicate of this bug: 1965437
Duplicate of this bug: 1960620
Duplicate of this bug: 1945452
Depends on: 1976026
Blocks: 1978075
Regressions: 1800223
See Also: → 1978353
No longer depends on: 668209
See Also: → 668209
No longer depends on: 681392
See Also: → 681392
Blocks: 1862935
No longer depends on: 1862935
No longer blocks: 1978075
Depends on: 1978075
Blocks: 1706538
Depends on: 1987344
See Also: → 1989056
Blocks: 1782501
User Story: (updated)
Depends on: 1992762
Depends on: 1994668
Depends on: 1996054
User Story: (updated)
See Also: → 1996385
Blocks: 1717403
User Story: (updated)
See Also: → 2000283
Duplicate of this bug: 1996054
Depends on: 1775609
See Also: → 2001583

The conceptual change is small but it requires several test changes.

In principle, tests cannot open a new empty window, modify the global, then
load their target document and expect their modifications to stick. Only if the
target URI is opened directly, the synchronously available window is reused. So
either tests need to do that, or rely on messages.

Duplicate of this bug: 1945808
Duplicate of this bug: 1949168
Duplicate of this bug: 1859484
Blocks: 1979032
Pushed by vhilla@mozilla.com: https://github.com/mozilla-firefox/firefox/commit/39a8a4171bb2 https://hg.mozilla.org/integration/autoland/rev/5f9e366c17fc Make initial about:blank not get overwritten by an async about:blank load. r=sessionstore-reviewers,sfoster,timhuang,credential-management-reviewers,issammani,webidl,extension-reviewers,tabbrowser-reviewers,migration-reviewers,home-newtab-reviewers,hsivonen,cookie-reviewers,fxview-reviewers,firefox-desktop-core-reviewers ,dao,valentin,mossop,smaug,geckoview-reviewers,Jamie,mtigley,thecount,media-playback-reviewers,padenot,dom-worker-reviewers,jesup,jdescottes,asuth,robwu,karlt,profiler-reviewers,kpatenio,devtools-reviewers,tcampbell,nchevobbe https://github.com/mozilla-firefox/firefox/commit/967fc6d445e4 https://hg.mozilla.org/integration/autoland/rev/8972ceeb1559 Ensure the initial about:blank SH entry is replaced. r=smaug https://github.com/mozilla-firefox/firefox/commit/9812cb264720 https://hg.mozilla.org/integration/autoland/rev/86c4f03a4305 Only reuse inner window for uncommitted initial documents. r=smaug,asuth,credential-management-reviewers,webrtc-reviewers,pehrsons,hsivonen,dimi
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/56213 for changes under testing/web-platform/tests
Regressions: 2001934
No longer regressions: 2001934
Regressions: 2001930
Regressions: 2001935
Regressions: 2001934
Attachment #9528660 - Attachment is obsolete: true
Regressions: 2001963

https://hg.mozilla.org/mozilla-central/rev/5f9e366c17fc
https://hg.mozilla.org/mozilla-central/rev/8972ceeb1559
https://hg.mozilla.org/mozilla-central/rev/86c4f03a4305

Status: ASSIGNED → RESOLVED
Closed: 5 days ago
status-firefox147: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 147 Branch
Upstream PR merged by moz-wptsync-bot
Regressions: 2002058
See Also: → 2002069
Regressions: 2002073
Blocks: 2002189
Regressions: 2002313
See Also: → 2002326
Regressions: 1899595
Regressions: 2002481
Regressions: 2002520
Regressions: 2002544
Regressions: 2002654
See Also: → 2002721
Regressions: 2002767
Regressions: 2002870
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: