827870 - Assertion when setting expando on a document xray

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Boris Zbarsky [:bzbarsky]]

If I run the js/xpconnect/tests/chrome/test_bug738244.xul test, I get an assertion:

#0  NS_DebugBreak_P (aSeverity=1, aStr=0x105e58c00 "trying to wrap a holder", aExpr=0x105e58c18 "JS_GetClass(obj) != &XrayUtils::HolderClass", aFile=0x105e58aa4 "../../../../mozilla/js/xpconnect/wrappers/WrapperFactory.cpp", aLine=328) at nsDebugImpl.cpp:285
#1  0x00000001041623b9 in xpc::WrapperFactory::Rewrap (cx=0x11597ef50, existing=0x0, obj=0x128249280, wrappedProto=0x1, parent=0x10f141060, flags=0) at WrapperFactory.cpp:328
#2  0x00000001010b0373 in JSCompartment::wrap (this=0x12658e000, cx=0x11597ef50, vp=0x7fff5fbf72b8, existing=0x0) at jscompartment.cpp:401
#3  0x00000001010b08fe in JSCompartment::wrap (this=0x12658e000, cx=0x11597ef50, objp=0x7fff5fbf7388, existing=0x0) at jscompartment.cpp:443
#4  0x00000001010b0db1 in JSCompartment::wrap (this=0x12658e000, cx=0x11597ef50, desc=0x7fff5fbf7388) at jscompartment.cpp:483
#5  0x00000001010ec125 in JS_WrapPropertyDescriptor (cx=0x11597ef50, desc=0x7fff5fbf7388) at jsfriendapi.cpp:247
#6  0x0000000104157a3a in xpc::XrayWrapper<js::CrossCompartmentWrapper, xpc::XPCWrappedNativeXrayTraits>::defineProperty (this=0x1071b97f0, cx=0x11597ef50, wrapper=0x10fc02180, id={asBits = 4554045312}, desc=0x7fff5fbf76c0) at XrayWrapper.cpp:1489
#7  0x00000001011fa5e1 in js::BaseProxyHandler::set (this=0x1071b97f0, cx=0x11597ef50, proxy_=0x10fc02180, receiver_=0x10fc02180, id_={asBits = 4554045312}, strict=false, vp=0x7fff5fbf7c68) at jsproxy.cpp:203
#8  0x0000000104158014 in xpc::XrayWrapper<js::CrossCompartmentWrapper, xpc::XPCWrappedNativeXrayTraits>::set (this=0x1071b97f0, cx=0x11597ef50, wrapper=0x10fc02180, receiver=0x10fc02180, id={asBits = 4554045312}, strict=false, vp=0x7fff5fbf7c68) at XrayWrapper.cpp:1598

from this line of the test:

        doc.getElementById = 42;

Not sure whether this is a regression or not, but it doesn't look good.  Too bad mochitests don't fail on asserts.  :(

Bobby, do you know what's going on here?

Comment 1

[Andrew McCreight [:mccr8]]

That sounds vaguely similar to bug 826471 comment 3.

Comment 2

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #0)
> Bobby, do you know what's going on here?

Yeah. I've fixed it and made the assertion fatal in bug 823348 part 4, which has been waiting on review for several weeks now (presumably due to b2g). :-(

Comment 3

[Daniel Veditz [:dveditz]]

There's such a lot of stuff in bug 823348 that a depends on link isn't giving much hint to this bug.

Comment 4

[Andrew McCreight [:mccr8]]

The blocking bug landed, and made this assert fatal, so it should be okay now on 21.  Do we know what other branches it affects?

Comment 5

[Daniel Veditz [:dveditz]]

Bobby: what part of 823348 specifically fixed it, and what branches had this problem? Do we need to backport something to ESR-17, b2g18, Firefox 20?

Comment 6

[Bobby Holley (:bholley)]

(In reply to Daniel Veditz [:dveditz] from comment #5)
> Bobby: what part of 823348 specifically fixed it, and what branches had this
> problem? Do we need to backport something to ESR-17, b2g18, Firefox 20?

Part 4 should fix the problem. It's hard to say offhand what's affected, but I think the best option is to just to backport that patch to those branches. The patch includes a fatal assertion to make sure we're getting it right.

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Please go ahead and nominate part 4 for uplift to aurora, esr17, and b2g18.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Do a better job of lying about the holder and make assertions fatal (on aurora). r=mrbkap

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 712876 [details] [diff] [review]
Do a better job of lying about the holder and make assertions fatal (on aurora). r=mrbkap

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown
User impact if declined: potential security issues, we don't know much more
Testing completed (on m-c, etc.): baked on m-c for several weeks
Risk to taking this patch (and alternatives if risky): not risky
String or UUID changes made by this patch: none

Comment 10

[Bobby Holley (:bholley)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3975c01ee426

Comment 11

[Bobby Holley (:bholley)]

Attachment: Do a better job of lying about the holder and make assertions fatal (on esr17 and b2g18). r=mrbkap

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 712969 [details] [diff] [review]
Do a better job of lying about the holder and make assertions fatal (on esr17 and b2g18). r=mrbkap

See comment 9.

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Batch edit: Bugs still affected on b2g18 after 2/13 merge to v1.0.1 branch are affected on v1.0.1 branch.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/9968e83f2959
https://hg.mozilla.org/releases/mozilla-b2g18/rev/a52af6b512d4

It's not clear to me whether this was approved to land on b2g18_v1_0_1 at this time or not, so I didn't land it there. Please set checkin-needed if it needs uplift there as well and I'll get it.

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Yes, this does need to go onto v1_0_1, thanks for checking.

Comment 16

[Bobby Holley (:bholley)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #14)
> It's not clear to me whether this was approved to land on b2g18_v1_0_1 at
> this time or not, so I didn't land it there. Please set checkin-needed if it
> needs uplift there as well and I'll get it.

As that whole distinction is beyond me at the moment, RyanVM++ for taking care of this. :-)

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/ad6e22bd7a6a