697833 - Bad assertions with html5.parser.enable=false, designMode, document.write

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Jesse Ruderman]

Attachment: testcase (requires html5.parser.enable set to false)

###!!! ASSERTION: root element frame already created: 'nsnull == mRootElementFrame', file layout/base/nsCSSFrameConstructor.cpp, line 6885

  ... various other assertions ...

when closing:

###!!! ASSERTION: Some pres arena objects were not freed: 'mPresArenaAllocCount == 0', file layout/base/nsPresShell.cpp, line 980

Comment 1

[Jesse Ruderman]

Attachment: more assertions and stack traces

Comment 2

[Jesse Ruderman]

Does this just mean we should should remove the html5.parser.enable pref?

Comment 3

[Henri Sivonen (:hsivonen)]

(In reply to Jesse Ruderman from comment #2)
> Does this just mean we should should remove the html5.parser.enable pref?

I don't know if the assertions you are seeing are revealing real bugs in layout, but the html5.parser.enable pref has been treated "flip at your own risk" insecure per discussion around the time when we decided to leave the pref in Firefox 4.

I'd be OK with removing the pref. EMC appears to have been telling their customers to flip the pref, so who knows how many Firefox installations have it flipped. (EMC has since then fixed their problem that inspired them to tell their customers to flip the pref.)

Comment 4

[Henri Sivonen (:hsivonen)]

Attachment: Stop honoring the pref

This patch stops honoring the pref and removes the pref form all.js. I'm making a minimal patch at first so that the patch is landable on branches with minimum risk. (Obviously, this patch leaves some dead code in Gecko that needs to be cleaned up.)

Using the tryserver now to find out if we still have tests that flip the pref and fail if the pref isn't honored anymore. (MXR says we don't.)

Comment 5

[Henri Sivonen (:hsivonen)]

Comment on attachment 570204 [details] [diff] [review]
Stop honoring the pref

Let's get this landed on trunk and then seek approval for aurora and beta to prevent users shooting themselves in the foot by following advice to turn the HTML5 parser off.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 570204 [details] [diff] [review]
Stop honoring the pref

Yes. HTML5 parser has been enabled now in, hmm, 3 releases. That should
have given enough confidence that it is working well.

Comment 7

[Henri Sivonen (:hsivonen)]

(In reply to Olli Pettay [:smaug] from comment #6)
> Yes. HTML5 parser has been enabled now in, hmm, 3 releases.

4 actually. :-)

Thanks for the r+:
https://hg.mozilla.org/integration/mozilla-inbound/rev/78ffa0184082

> That should have given enough confidence that it is working well.

The pref wasn't still around for lack of confidence but for ease of regression testing.

Comment 8

[Henri Sivonen (:hsivonen)]

Comment on attachment 570204 [details] [diff] [review]
Stop honoring the pref

Requesting approval for aurora and beta:
 * The patch is extremely simple. It removes configurability but doesn't change the default configuration that we've shipped in Firefox 4, 5, 6 and 7.
 * The benefit is that users who've listened to bad advice wouldn't be running potentially insecure code and lacking the newest Firefox features.
 * The risk is that an enterprise somewhere is using rapid releases even if their systems don't work with the HTML5 parser and have turned the HTML5 parser off, so if they have been testing beta or aurora and we now put this is beta, they'll have a short notice for the pref going away.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/78ffa0184082

Comment 10

[Henri Sivonen (:hsivonen)]

For clarity: The now-fixed EMC problem mentioned in comment 3 was on EMC's extranet. It was not in a product shipped to customers, so there's no need to wait for their customers to upgrade a product or anything like that.

Comment 11

[LegNeato]

Comment on attachment 570204 [details] [diff] [review]
Stop honoring the pref

[triage comment]

The benefit of this seems low. We'll let it come up through the normal process.

Comment 12

[Jesse Ruderman]

> The benefit of this seems low

fwiw, about half of "Some pres arena objects were not freed" bugs turn out to be exploitable.

Comment 13

[Daniel Veditz [:dveditz]]

Is there a follow-up "Rip out the old parser dead code" bug? If so it should be mentioned/linked from this bug. Not exactly a dependency, maybe the "See Also" field?

Comment 14

[Jesse Ruderman]

There's a set of bugs, including bug 651045.