Open Bug 543435 (sync-about-blank) Opened 15 years ago Updated 11 months ago

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

Categories

(Core :: DOM: Navigation, defect)

Product:
Core
Component:
DOM: Navigation
Type:
defect

Tracking

()

Status:
ASSIGNED
Project Flags:
Webcompat Priority P2

People

(Reporter: hsivonen, Assigned: hsivonen, NeedInfo)

References

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

Details

(Keywords: html5)

Attachments

(1 file, 12 obsolete files)

Bug 543435 - Make initial about:blank not get overwritten by an async about:blank load.
(deleted), text/x-phabricator-request
Details 
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
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)
Attached patch Make the initial about:blank synchronous (obsolete) (deleted) — — Splinter Review 
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)
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.
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
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.
Attached patch Make the initial about:blank synchronous, v7 (obsolete) (deleted) — — Splinter Review 
Still orange but works for dog food.
Attachment #562404 - Attachment is obsolete: true
Depends on: 779959
No longer depends on: 779430
Blocks: 473396
Assignee: hsivonen → nobody
Status: ASSIGNED → NEW
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
No longer depends on: 1724975
Webcompat Priority: --- → ?
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

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.

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)

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

/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.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: