533381 - [HTML5] Use the old parser for about:blank

Status: RESOLVED FIXED

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

Description

[Henri Sivonen (:hsivonen)]

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?

Comment 1

[Henri Sivonen (:hsivonen)]

Works on http://ted.mielczarek.org/code/mozilla/mochitest-maker/ ...

Comment 2

[Henri Sivonen (:hsivonen)]

Suspected cause: about:blank not loading synchronously.

Comment 3

[Henri Sivonen (:hsivonen)]

Attachment: Call EnsureContentViewer() for about:blank

Not sure this is sufficient or if it's necessary to do more.

Comment 4

[Henri Sivonen (:hsivonen)]

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

Comment 5

[Henri Sivonen (:hsivonen)]

content/base/test/test_bug500937.html also fails for this reason.

Comment 6

[Henri Sivonen (:hsivonen)]

Also content/html/document/test/test_bug486741.html

Comment 7

[Henri Sivonen (:hsivonen)]

There's a similar problem with data: URLs, too: bug 539896.

Comment 8

[Henri Sivonen (:hsivonen)]

Attachment: Generate initial about:blank synchronously and fire onload for it

Comment 9

[Henri Sivonen (:hsivonen)]

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. :-(

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.

Comment 11

[Henri Sivonen (:hsivonen)]

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

> 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!

Comment 13

[Henri Sivonen (:hsivonen)]

Attachment: Use the old parser for about:blank even if html5.enable=true

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

Comment 14

[Henri Sivonen (:hsivonen)]

Bug 543435 is the new bug.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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.

Comment 16

[Henri Sivonen (:hsivonen)]

Pushed with review comments addressed. Thanks.
http://hg.mozilla.org/mozilla-central/rev/1de016190974