Closed
Bug 745494
Opened 12 years ago
Closed 12 years ago
"ASSERTION: Didn't restore state properly?"
Categories
(Core :: DOM: Editor, defect)
Tracking
()
People
(Reporter: jruderman, Assigned: ehsan.akhgari)
References
Details
(Keywords: assertion, regression, testcase, Whiteboard: [sg:critical][qa+][advisory-tracking+])
Attachments
(4 files)
308 bytes,
text/html
|
Details | |
15.01 KB,
text/plain
|
Details | |
1.27 KB,
patch
|
roc
:
review+
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta-
lsblakk
:
approval-mozilla-esr10+
|
Details | Diff | Splinter Review |
47 bytes,
text/x-phabricator-request
|
Details | Review |
###!!! ASSERTION: Please remove this from the document properly: '!IsInDoc()', file content/base/src/nsGenericElement.cpp, line 2509 This assertion is covered by bug 718282. ###!!! ASSERTION: Didn't restore state properly?: 'mSubtreeRoot == this', file content/base/src/nsGenericElement.cpp, line 210 This assertion was added in bug 730587.
Reporter | ||
Comment 1•12 years ago
|
||
This is bad.
Group: core-security
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → ehsan
Assignee | ||
Comment 4•12 years ago
|
||
So, here's an analysis of what happens here. The element which gets destroyed before being removed from the doc is the inline table editing UI. We destroy these elements in a very gross way (see bug 439258), explicitly from nsHTMLEditor::~nsHTMLEditor. In order to destroy these elements, we need the root element (mRootElement) which serves as the parent for these elements, but that gets cleared in nsPlaintextEditor::PreDestroy, which results us to fail to destroy these elements properly, so they will remain in the document. This is pretty bad, but not as bad as what it seems like at first. These are native anonymous elements, so the web content cannot directly access them. But still this may be used in addition to other stuff to come up with an exploit. So, I think sg:crit is the right classification. This has been broken since the inception of table/abs-pos editing UI, and infects all versions of Gecko. We should also consider this if we decide to ship another 1.9.2 version.
blocking1.9.2: --- → ?
status1.9.2:
--- → ?
status-firefox-esr10:
--- → affected
status-firefox11:
--- → affected
status-firefox12:
--- → affected
status-firefox13:
--- → affected
status-firefox14:
--- → affected
tracking-firefox-esr10:
--- → ?
tracking-firefox11:
--- → ?
tracking-firefox12:
--- → ?
tracking-firefox13:
--- → ?
tracking-firefox14:
--- → ?
Flags: in-testsuite?
Assignee | ||
Comment 5•12 years ago
|
||
Fortunately, the fix is very easy and extremely safe. [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: It is sg:crit. [Approval Request Comment] Regression caused by (bug #): Broken since the dawn of time. User impact if declined: Could be exploitable. Testing completed (on m-c, etc.): Tested by code inspection and making sure that the patch fixes the assertion in the testcase. Risk to taking this patch (and alternatives if risky): The patch is very small and extremely safe. String changes made by this patch: None. [Approval Request Comment] Regression caused by (bug #): Broken since the dawn of time. User impact if declined: Could be exploitable. Testing completed (on m-c, etc.): Tested by code inspection and making sure that the patch fixes the assertion in the testcase. Risk to taking this patch (and alternatives if risky): The patch is very small and extremely safe. String changes made by this patch: None.
Attachment #616365 -
Flags: review?(roc)
Attachment #616365 -
Flags: approval-mozilla-esr10?
Attachment #616365 -
Flags: approval-mozilla-beta?
Attachment #616365 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 6•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=5fab1b4c5e7f
Comment on attachment 616365 [details] [diff] [review] Patch (v1) Review of attachment 616365 [details] [diff] [review]: ----------------------------------------------------------------- Don't forget to add the test later.
Attachment #616365 -
Flags: review?(roc) → review+
Assignee | ||
Comment 8•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/559cd0ff610c
Target Milestone: --- → mozilla14
Assignee | ||
Comment 9•12 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7) > Comment on attachment 616365 [details] [diff] [review] > Patch (v1) > > Review of attachment 616365 [details] [diff] [review]: > ----------------------------------------------------------------- > > Don't forget to add the test later. Sure, the in-testsuite? flag will remind me. :-)
Assignee | ||
Comment 10•12 years ago
|
||
"Hide the anonymous editing UI before its too late; r=roc a=blocking". ^^^ Time to learn some English! ;-)
Assignee | ||
Comment 11•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/559cd0ff610c
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 12•12 years ago
|
||
Comment on attachment 616365 [details] [diff] [review] Patch (v1) [Triage Comment] We discussed this in today's channel meeting. We're at the point in this release where we're only taking fixes that have some probability of being a major issue over the next 6 weeks. We feel that the probability here is low enough that we'd like to take this first in FF13. Approved for Aurora.
Attachment #616365 -
Flags: approval-mozilla-beta?
Attachment #616365 -
Flags: approval-mozilla-beta-
Attachment #616365 -
Flags: approval-mozilla-aurora?
Attachment #616365 -
Flags: approval-mozilla-aurora+
Updated•12 years ago
|
blocking1.9.2: ? → ---
status1.9.2:
? → ---
Assignee | ||
Comment 13•12 years ago
|
||
http://hg.mozilla.org/releases/mozilla-aurora/rev/e13879b89d80
Comment 14•12 years ago
|
||
Comment on attachment 616365 [details] [diff] [review] Patch (v1) [Triage Comment] Since this is on 13, please go ahead and land to ESR (Firefox 13).
Attachment #616365 -
Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Assignee | ||
Comment 15•12 years ago
|
||
http://hg.mozilla.org/releases/mozilla-esr10/rev/f7578a09b384
Updated•12 years ago
|
Whiteboard: [sg:critical][qa+] → [sg:critical][qa+][advisory-tracking+]
Updated•12 years ago
|
Verified on Nightly, 10esr and 13 beta on 10.7
Status: RESOLVED → VERIFIED
Updated•12 years ago
|
Group: core-security
Assignee | ||
Updated•4 years ago
|
Flags: in-testsuite? → in-testsuite+
Assignee | ||
Comment 17•4 years ago
|
||
Comment 18•4 years ago
|
||
Pushed by eakhgari@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/91c04bc7ec99 Add a crashtest based on the test case for the bug
Comment 19•4 years ago
|
||
bugherder |
Comment 20•4 years ago
|
||
Bugbug thinks this bug is a regression, but please revert this change in case of error.
Keywords: regression
You need to log in
before you can comment on or make changes to this bug.
Description
•