Closed Bug 840098 Opened 11 years ago Closed 11 years ago

"ASSERTION: Unexpected aDocument" after adopting content from XBL


(Core :: XBL, defect)

Not set



Tracking Status
firefox21 - wontfix
firefox22 + verified
firefox23 + verified
firefox-esr17 22+ verified
b2g18 ? fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed


(Reporter: jruderman, Assigned: mrbkap)



(5 keywords, Whiteboard: [land test on 8/5][adv-main22+][adv-esr1707+])


(5 files, 1 obsolete file)

Attached file testcase
###!!! ASSERTION: Unexpected aDocument: 'aDocument == mDocument', file layout/base/nsPresShell.cpp, line 4073

###!!! ASSERTION: Should be in an update while creating frames: 'mUpdateCount != 0', file layout/base/nsCSSFrameConstructor.cpp, line 6532

And a memory leak.

(Security-sensitive because the second assertion has appeared in security bugs.)
Attached file stacks
Jesse, is this a regression from bug 821850? Does it depend on the pref being flipped?
This happens with a clean profile / default prefs.  I don't know whether it's a regression.
Ok, I think this is unrelated to my XBL stuff. Though it sounds somewhat like what blake was looking at last week. Blake, is this related?
Flags: needinfo?(mrbkap)
David, we're unsure how to sec- rate this one. How gloomy is the nsCSSFrameConstructor.cpp assert?
Flags: needinfo?(dbaron)
Both assertions sound like things have gone pretty wrong in a way that I could imagine being exploitable, but I can't really make a useful guess about the likelihood/danger off the top of my head.  I think it's one of those things where we'll know more once we figure out the fix.
Flags: needinfo?(dbaron)
OK, guessing sec-high to split the difference between critical and moderate.
Keywords: sec-high
It's unlikely that this is directly related to what I was looking at, but I can definitely investigate here.
Assignee: nobody → mrbkap
Flags: needinfo?(mrbkap)
Attached patch Proposed fixSplinter Review
The root of an anonymous subtree is special in that it is not in its parent's child list. Moving such a root out from under its shadow parent confuses the XBL code (and possibly also the BindToTree code). This patch fixes adoptNode to follow nsINode::ReplaceOrInsertBefore's behavior by making sure we don't move the root of an anonymous subtree out from under it.
Attachment #728488 - Flags: review?(bzbarsky)
Comment on attachment 728488 [details] [diff] [review]
Proposed fix

Attachment #728488 - Flags: review?(bzbarsky) → review+
Comment on attachment 728488 [details] [diff] [review]
Proposed fix

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

The patch points directly at the problem (calling adopt node on the root of an anonymous subtree causes problems). However, I don't think that anybody actually knows how to exploit this. We know that it's dangerous, so we're treating it as sec-high, but coming up with an actual exploit is likely many hours worth of work.

Which older supported branches are affected by this flaw?

Some digging around showed that bug 650493 is at fault here (it removed an implicit check for the adopted node being in the child list of the parent).

How likely is this patch to cause regressions; how much testing does it need?

This is extremely unlikely to cause regressions. It introduces an error check that bails out only in cases that would cause this bug.
Attachment #728488 - Flags: sec-approval?
Comment on attachment 728488 [details] [diff] [review]
Proposed fix

sec-approval+ but only for a checkin 4/9 or after.
Attachment #728488 - Flags: sec-approval? → sec-approval+
Attached patch For checkinSplinter Review
This makes life a little clearer for the future with an additional assertion (r=sicking if anybody's tracking it).
Attachment #736049 - Flags: review+
Keywords: checkin-needed
Closed: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Late in the game to get this into Firefox 21, let's just wait for 22.
Blake - is this ready for uplift to FF22?
Flags: needinfo?(mrbkap)
Comment on attachment 736049 [details] [diff] [review]
For checkin

Attachment #736049 - Flags: approval-mozilla-beta?
Flags: needinfo?(mrbkap)
Attachment #736049 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Can we get the esr patch as well ?
Attached patch esr17 patch (obsolete) — Splinter Review
Attachment #753002 - Flags: approval-mozilla-esr17?
Attached patch esr17 patchSplinter Review
Sorry, the previous esr17 patch didn't compile.
Attachment #753002 - Attachment is obsolete: true
Attachment #753002 - Flags: approval-mozilla-esr17?
Attachment #753041 - Flags: approval-mozilla-esr17?
Verified as fixed on the 05/29 beta debug build on Mac OSX 10.7.5. No assertions are displayed with the attached test case anymore and the test case is displayed fine.
Attachment #753041 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17+
Confirmed assertion 17.0.6esr.
Confirmed fixed 17esr 2013-06-18.
Confirmed fixed FF23 2013-06-14.
Whiteboard: [land test on 8/5]
Whiteboard: [land test on 8/5] → [land test on 8/5][adv-main22+][adv-esr1707+]
This doesn't seem to have made it to b2g18, although that's marked "affected".
tracking-b2g18: --- → ?

a=bajaj over email. Did this test ever land? Looks like it was supposed to back in August.
Flags: needinfo?(mrbkap)
...and that was the wrong testcase, oops.
Flags: in-testsuite? → in-testsuite+
Group: core-security
You need to log in before you can comment on or make changes to this bug.