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

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

Categories

(Core :: Document Navigation, defect)

defect
Not set

Tracking

()

People

(Reporter: hsivonen, Unassigned)

References

(Depends on 2 open bugs, Blocks 6 open bugs, )

Details

(Keywords: html5)

Attachments

(1 file, 10 obsolete files)

35.94 KB, patch
Details | Diff | Splinter 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
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
Duplicate of this bug: 1074549
Duplicate of this bug: 837074
Assignee: hsivonen → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.