Closed Bug 533381 Opened 10 years ago Closed 10 years ago

[HTML5] Use the old parser for about:blank

Categories

(Core :: HTML: Parser, defect, P1)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: hsivonen, Assigned: hsivonen)

References

Details

Attachments

(1 file, 2 obsolete files)

content/html/document/test/test_bug404320.html fails due to document.body being null. 

Maybe the iframe doesn't have a full about:blank-like document?
Suspected cause: about:blank not loading synchronously.
Blocks: 510802
Summary: [HTML5] content/html/document/test/test_bug404320.html fails → [HTML5] about:blank not loaded synchronously into frames
Not sure this is sufficient or if it's necessary to do more.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Not surprisingly, the patch isn't sufficient. The load/unload/pageshow events don't appear as expect in http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/docshell/test/chrome/bug112564_window.xul
content/base/test/test_bug500937.html also fails for this reason.
Also content/html/document/test/test_bug486741.html
There's a similar problem with data: URLs, too: bug 539896.
See Also: → 539896
Blocks: 539915
Blocks: 540480
Attachment #419905 - Attachment is obsolete: true
The new patch is a failure, too:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1264767726.1264774981.27800.gz

I'm getting a feeling that it would make sense to remove this from the critical path of the HTML5 parser by creating a dummy nsIParser (that creates the about:blank DOM immediately upon OnStopRequest) for about:blank when html5.enable=true and leave the real spec compliant fix for later.

Boris, what's your opinion? Should I fix this per Hixie's draft on the critical path or for now just paper over this with an nsIParser impl. that participates on the current load and event dispatching code paths? It seems to me that fixing this per spec is another Mochitest whack-a-mole project in itself. :-(
Yeah, our tests do all sorts of things to work around the async about:blank load.  

That said, have you tried doing the patch here _and_ not starting a load in the frameloader?  As in, something closer to what hixie's spec actually suggests?  If that passes (which seems more likely than the patch attached here passing), we can work on removing this extra bogus onload and progress notifications as a separate followup.

I would be ok with a separate nsIParser if it's _very_ safe and if there's a deadline on removing it.
(In reply to comment #10)
> Yeah, our tests do all sorts of things to work around the async about:blank
> load.  
> 
> That said, have you tried doing the patch here _and_ not starting a load in the
> frameloader?

I don't follow. If the frame loader doesn't start the load far enough to reach the code in this patch, the code in this patch wouldn't run (or so I think).

> As in, something closer to what hixie's spec actually suggests? 

Hixie suggests not firing onload at all for the initial about:blank even if the initial about:blank isn't immediately navigated away from, which seems like a very probably Web compat problem and is (tried it) a sure test suite problem.

> I would be ok with a separate nsIParser if it's _very_ safe and if there's a
> deadline on removing it.

Actually, if another nsIParser is a temporary band-aid, nsParser could be it...
> If the frame loader doesn't start the load far enough to reach the code in this
> patch,

Oh, I see. The lack of context and lack of -p confused me as to what code you were changing.

> Hixie suggests not firing onload at all for the initial about:blank even if
> the initial about:blank isn't immediately navigated away from

Yes.

> And is (tried it) a sure test suite problem.

Right; see comment 10 paragraph 1.

> Actually, if another nsIParser is a temporary band-aid, nsParser could be 
> it...

Sold!
(In reply to comment #12)
> > Hixie suggests not firing onload at all for the initial about:blank even if
> > the initial about:blank isn't immediately navigated away from
> 
> Yes.

Not firing the events scares me, since the major browsers currently fire the 'load' event for all URLs. 

> > And is (tried it) a sure test suite problem.
> 
> Right; see comment 10 paragraph 1.

I'll file a new bug about tweaking attachment 424224 [details] [diff] [review] and the tests enough to make that approach landable.

> > Actually, if another nsIParser is a temporary band-aid, nsParser could be 
> > it...
> 
> Sold!

Patch attached.
Attachment #424224 - Attachment is obsolete: true
Attachment #424564 - Flags: review?(bzbarsky)
Bug 543435 is the new bug.
Summary: [HTML5] about:blank not loaded synchronously into frames → [HTML5][Patch] about:blank not loaded synchronously into frames
Comment on attachment 424564 [details] [diff] [review]
Use the old parser for about:blank even if html5.enable=true

>+  if (loadAsHtml5 && !(contentType.EqualsLiteral("text/html") && 
>+      aCommand && !nsCRT::strcmp(aCommand, "view"))) {

That's parenthesized and indented really confusingly. Please put a linebreak after the first && and line things up under that.

r=bzbarsky with that and a FIXME comment pointing to a followup bug on sorting this out.
Attachment #424564 - Flags: review?(bzbarsky) → review+
Pushed with review comments addressed. Thanks.
http://hg.mozilla.org/mozilla-central/rev/1de016190974
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Summary: [HTML5][Patch] about:blank not loaded synchronously into frames → [HTML5] Use the old parser for about:blank
Duplicate of this bug: 539915
No longer blocks: 540480
You need to log in before you can comment on or make changes to this bug.