Closed
Bug 697833
Opened 13 years ago
Closed 13 years ago
Bad assertions with html5.parser.enable=false, designMode, document.write
Categories
(Core :: DOM: Navigation, defect)
Tracking
()
RESOLVED
FIXED
mozilla10
Tracking | Status | |
---|---|---|
status1.9.2 | --- | ? |
People
(Reporter: jruderman, Assigned: hsivonen)
References
Details
(Keywords: assertion, memory-leak, testcase)
Attachments
(3 files)
(deleted),
text/html
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
smaug
:
review+
christian
:
approval-mozilla-aurora-
christian
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
###!!! 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
Reporter | ||
Comment 1•13 years ago
|
||
Reporter | ||
Comment 2•13 years ago
|
||
Does this just mean we should should remove the html5.parser.enable pref?
Assignee | ||
Comment 3•13 years ago
|
||
(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.)
Assignee | ||
Comment 4•13 years ago
|
||
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.)
Assignee | ||
Comment 5•13 years ago
|
||
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 6•13 years ago
|
||
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+
Assignee | ||
Comment 7•13 years ago
|
||
(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.
Assignee | ||
Comment 8•13 years ago
|
||
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 | ||
Updated•13 years ago
|
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Assignee | ||
Updated•13 years ago
|
status1.9.2:
--- → ?
Comment 9•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Keywords: #relman/triage/defer-to-group
Assignee | ||
Comment 10•13 years ago
|
||
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•13 years ago
|
||
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-
Reporter | ||
Comment 12•13 years ago
|
||
> 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•13 years ago
|
||
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?
Reporter | ||
Comment 14•13 years ago
|
||
There's a set of bugs, including bug 651045.
Assignee | ||
Updated•13 years ago
|
Updated•10 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•