Open Bug 543435 (sync-about-blank) Opened 14 years ago Updated 15 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:
ASSIGNED
Project Flags:
Webcompat Priority P2

People

(Reporter: hsivonen, Assigned: hsivonen)

References

(Depends on 5 open bugs, Blocks 16 open bugs, Regressed 2 open bugs,
URL
)

Details

(Keywords: html5)

Attachments

(1 file, 12 obsolete files)

WIP: Bug 543435 - Make initial about:blank not get overwritten by an async about:blank load.
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

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: