Closed Bug 697833 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: DOM: Navigation, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10
Tracking Status
status1.9.2 --- ?

People

(Reporter: jruderman, Assigned: hsivonen)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, memory-leak, testcase)

Attachments

(3 files)

###!!! 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
Does this just mean we should should remove the html5.parser.enable pref?
(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.)
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 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.
Attachment #570204 - Flags: review?(Olli.Pettay)
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.
Attachment #570204 - Flags: review?(Olli.Pettay) → review+
(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 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.
Attachment #570204 - Flags: approval-mozilla-beta?
Attachment #570204 - Flags: approval-mozilla-aurora?
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
status1.9.2: --- → ?
https://hg.mozilla.org/mozilla-central/rev/78ffa0184082
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
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 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.
Attachment #570204 - Flags: approval-mozilla-beta?
Attachment #570204 - Flags: approval-mozilla-beta-
Attachment #570204 - Flags: approval-mozilla-aurora?
Attachment #570204 - Flags: approval-mozilla-aurora-
> The benefit of this seems low

fwiw, about half of "Some pres arena objects were not freed" bugs turn out to be exploitable.
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?
There's a set of bugs, including bug 651045.
See Also: → 563890
Group: core-security
You need to log in before you can comment on or make changes to this bug.