840098 - "ASSERTION: Unexpected aDocument" after adopting content from XBL

Status: VERIFIED FIXED

(Core :: XBL, defect)

Description

[Jesse Ruderman]

Attachment: 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.)

Comment 1

[Jesse Ruderman]

Attachment: stacks

Comment 2

[Bobby Holley (:bholley)]

Jesse, is this a regression from bug 821850? Does it depend on the pref being flipped?

Comment 3

[Jesse Ruderman]

This happens with a clean profile / default prefs.  I don't know whether it's a regression.

Comment 4

[Bobby Holley (:bholley)]

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?

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

David, we're unsure how to sec- rate this one. How gloomy is the nsCSSFrameConstructor.cpp assert?

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 7

[Daniel Veditz [:dveditz]]

OK, guessing sec-high to split the difference between critical and moderate.

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

Blake, any thoughts on comment 4?

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

It's unlikely that this is directly related to what I was looking at, but I can definitely investigate here.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 728488 [details] [diff] [review]
Proposed fix

r=me

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 13

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 728488 [details] [diff] [review]
Proposed fix

sec-approval+ but only for a checkin 4/9 or after.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: For checkin

This makes life a little clearer for the future with an additional assertion (r=sicking if anybody's tracking it).

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4187e42329b

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e4187e42329b

Comment 17

[Daniel Veditz [:dveditz]]

Late in the game to get this into Firefox 21, let's just wait for 22.

Comment 18

[Alex Keybl [:akeybl]]

Blake - is this ready for uplift to FF22?

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 736049 [details] [diff] [review]
For checkin

Yes.

Comment 20

[bhavana bajaj [:bajaj]]

Can we get the esr patch as well ?

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d72bff7df023

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: esr17 patch

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: esr17 patch

Sorry, the previous esr17 patch didn't compile.

Comment 24

[Ioana (away)]

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.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/a48e03b9d823

Comment 26

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed assertion 17.0.6esr.
Confirmed fixed 17esr 2013-06-18.
Confirmed fixed FF23 2013-06-14.

Comment 27

[Daniel Veditz [:dveditz]]

This doesn't seem to have made it to b2g18, although that's marked "affected".

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/9d624187768c

a=bajaj over email. Did this test ever land? Looks like it was supposed to back in August.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/9d624187768c

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47416c6a473a

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

...and that was the wrong testcase, oops.

Comment 32

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a301aa4f3e43

Comment 33

[Carsten Book [:Tomcat]]

fixed on central https://hg.mozilla.org/mozilla-central/rev/a301aa4f3e43