Last Comment Bug 745494 - "ASSERTION: Didn't restore state properly?"
: "ASSERTION: Didn't restore state properly?"
Status: VERIFIED FIXED
[sg:critical][qa+][advisory-tracking+]
: assertion, testcase
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- normal (vote)
: mozilla14
Assigned To: :Ehsan Akhgari
:
: Makoto Kato [:m_kato]
Mentors:
Depends on:
Blocks: 336383 730587
  Show dependency treegraph
 
Reported: 2012-04-14 17:15 PDT by Jesse Ruderman
Modified: 2012-06-21 11:52 PDT (History)
11 users (show)
ehsan: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
wontfix
+
affected
+
verified
+
verified
13+
verified


Attachments
testcase (requires focus) (308 bytes, text/html)
2012-04-14 17:15 PDT, Jesse Ruderman
no flags Details
stack trace (15.01 KB, text/plain)
2012-04-14 17:16 PDT, Jesse Ruderman
no flags Details
Patch (v1) (1.27 KB, patch)
2012-04-18 17:13 PDT, :Ehsan Akhgari
roc: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-04-14 17:15:57 PDT
Created attachment 615089 [details]
testcase (requires focus)

###!!! 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.
Comment 1 Jesse Ruderman 2012-04-14 17:16:18 PDT
Created attachment 615090 [details]
stack trace
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-15 03:47:58 PDT
This is bad.
Comment 3 Daniel Veditz [:dveditz] 2012-04-18 10:54:37 PDT
taking a guess at a translation for "bad"
Comment 4 :Ehsan Akhgari 2012-04-18 17:09:46 PDT
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.
Comment 5 :Ehsan Akhgari 2012-04-18 17:13:40 PDT
Created attachment 616365 [details] [diff] [review]
Patch (v1)

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.
Comment 6 :Ehsan Akhgari 2012-04-18 17:16:33 PDT
https://tbpl.mozilla.org/?tree=Try&rev=5fab1b4c5e7f
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-18 17:38:36 PDT
Comment on attachment 616365 [details] [diff] [review]
Patch (v1)

Review of attachment 616365 [details] [diff] [review]:
-----------------------------------------------------------------

Don't forget to add the test later.
Comment 9 :Ehsan Akhgari 2012-04-18 20:48:57 PDT
(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.  :-)
Comment 10 :Ehsan Akhgari 2012-04-18 20:55:09 PDT
"Hide the anonymous editing UI before its too late; r=roc a=blocking".
                                      ^^^
Time to learn some English!  ;-)
Comment 12 Alex Keybl [:akeybl] 2012-04-19 14:32:42 PDT
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.
Comment 14 Lukas Blakk [:lsblakk] use ?needinfo 2012-04-25 14:25:11 PDT
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).
Comment 16 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-06-02 11:15:12 PDT
Verified on Nightly, 10esr and 13 beta on 10.7

Note You need to log in before you can comment on or make changes to this bug.