Closed Bug 745494 Opened 12 years ago Closed 12 years ago

"ASSERTION: Didn't restore state properly?"

Categories

(Core :: DOM: Editor, defect)

x86_64
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla14
Tracking Status
firefox11 + wontfix
firefox12 + affected
firefox13 + verified
firefox14 + verified
firefox-esr10 13+ verified

People

(Reporter: jruderman, Assigned: ehsan.akhgari)

References

Details

(Keywords: assertion, regression, testcase, Whiteboard: [sg:critical][qa+][advisory-tracking+])

Attachments

(4 files)

###!!! 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.
Attached file stack trace
This is bad.
Group: core-security
taking a guess at a translation for "bad"
Whiteboard: [sg:critical]
Assignee: nobody → ehsan
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: --- → ?
Flags: in-testsuite?
Attached patch Patch (v1)Splinter Review
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?
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+
(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.  :-)
"Hide the anonymous editing UI before its too late; r=roc a=blocking".
                                      ^^^
Time to learn some English!  ;-)
https://hg.mozilla.org/mozilla-central/rev/559cd0ff610c
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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+
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+
Whiteboard: [sg:critical] → [sg:critical][qa+]
Whiteboard: [sg:critical][qa+] → [sg:critical][qa+][advisory-tracking+]
Verified on Nightly, 10esr and 13 beta on 10.7
Status: RESOLVED → VERIFIED
Group: core-security
Flags: in-testsuite? → in-testsuite+
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/91c04bc7ec99
Add a crashtest based on the test case for the bug

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.

Attachment

General

Creator:
Created:
Updated:
Size: