Closed
Bug 745494
Opened 13 years ago
Closed 13 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•13 years ago
|
||
This is bad.
Group: core-security
| Assignee | ||
Updated•13 years ago
|
Assignee: nobody → ehsan
| Assignee | ||
Comment 4•13 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•13 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•13 years ago
|
||
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•13 years ago
|
||
Target Milestone: --- → mozilla14
| Assignee | ||
Comment 9•13 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•13 years ago
|
||
"Hide the anonymous editing UI before its too late; r=roc a=blocking".
^^^
Time to learn some English! ;-)
| Assignee | ||
Comment 11•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 12•13 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•13 years ago
|
blocking1.9.2: ? → ---
status1.9.2:
? → ---
| Assignee | ||
Comment 13•13 years ago
|
||
Comment 14•13 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•13 years ago
|
||
Updated•13 years ago
|
Whiteboard: [sg:critical][qa+] → [sg:critical][qa+][advisory-tracking+]
Updated•13 years ago
|
Verified on Nightly, 10esr and 13 beta on 10.7
Status: RESOLVED → VERIFIED
Updated•13 years ago
|
Group: core-security
| Assignee | ||
Updated•5 years ago
|
Flags: in-testsuite? → in-testsuite+
| Assignee | ||
Comment 17•5 years ago
|
||
Comment 18•5 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•5 years ago
|
||
| bugherder | ||
Comment 20•5 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
•