Last Comment Bug 761831 - "ASSERTION: obj must be a wrapper" with document.open during DOMNodeRemoved event
: "ASSERTION: obj must be a wrapper" with document.open during DOMNodeRemoved e...
Status: VERIFIED FIXED
[advisory-tracking+]
: assertion, testcase
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- normal (vote)
: mozilla17
Assigned To: Andrew McCreight [:mccr8]
: Anthony Hughes (:ashughes) [GFX][QA][Mentor]
Mentors:
Depends on:
Blocks: 325861
  Show dependency treegraph
 
Reported: 2012-06-05 15:43 PDT by Jesse Ruderman
Modified: 2012-10-21 22:13 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
verified
verified
verified
15+
verified


Attachments
testcase (366 bytes, text/html)
2012-06-05 15:43 PDT, Jesse Ruderman
no flags Details
stack trace (10.82 KB, text/plain)
2012-06-05 15:43 PDT, Jesse Ruderman
no flags Details
only generate error messages for simple cases (2.82 KB, patch)
2012-07-17 06:53 PDT, Andrew McCreight [:mccr8]
bobbyholley: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-06-05 15:43:38 PDT
Created attachment 630346 [details]
testcase

###!!! ASSERTION: obj must be a wrapper: 'IS_WRAPPER_CLASS(js::GetObjectClass(obj)) || js::GetObjectClass(obj) == &XPC_WN_Tearoff_JSClass', file js/xpconnect/src/XPCQuickStubs.cpp, line 374
Comment 1 Jesse Ruderman 2012-06-05 15:43:53 PDT
Created attachment 630348 [details]
stack trace
Comment 2 Daniel Veditz [:dveditz] 2012-06-06 10:15:11 PDT
Bobby: how bad is this assertion in terms of security bugs? Wonder if moz_bug_r_a4 can have some fun with this one.
Comment 3 Bobby Holley (:bholley) (busy with Stylo) 2012-06-06 16:19:10 PDT
I'm guessing this is the quickstubs version of bug 760887, which I'm hoping to look at soon. Might be a CPG regression, might not be. Not sure about the severity.
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2012-06-20 05:38:29 PDT
Passing over to Andrew since I think this is the same as bug 760887, which he's working on.

Andrew, if it's not, feel free to bounce this back to me.
Comment 5 Andrew McCreight [:mccr8] 2012-07-16 14:17:22 PDT
Once again, bholley's prediction about bugs being related is wrong. ;)

From looking at the test case and the code in nsINode::RemoveChild, I think what is happening is that we begin to remove a child of the document, which triggers the DOMNodeRemoved event, which resets the document using open().  Then we look for the child, but it isn't there any more, which causes us to return an error.  So far, that seems okay to me.  Olli, can you confirm that is what is supposed to happen here?

Anyways, once that happens the code tries to handle the error, and we end up in xpc_qsThrowMethodFailed to GetMethodInfo to GetMemberInfo.  The argument to GetMemberInfo is the document pointer of a window object, and is a proxy for the actual HTMLDocument reflector.  The code checks if the argument is a reflector, and it isn't, so it fails the assertion.

GetMemberInfo is pretty dusty looking code that is only called on error paths, so my guess would be that it just hasn't been updated for CPG and it needs to do some kind of unwrap thing.
Comment 6 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-07-16 15:58:40 PDT
(In reply to Andrew McCreight [:mccr8] from comment #5)
> Once again, bholley's prediction about bugs being related is wrong. ;)
> 
> From looking at the test case and the code in nsINode::RemoveChild, I think
> what is happening is that we begin to remove a child of the document, which
> triggers the DOMNodeRemoved event, which resets the document using open(). 
> Then we look for the child, but it isn't there any more, which causes us to
> return an error.  So far, that seems okay to me.  Olli, can you confirm that
> is what is supposed to happen here?
Sounds ok (I mean, that is what our implementation is supposed to do. There isn't a specification for that part).
Comment 7 Andrew McCreight [:mccr8] 2012-07-17 06:53:40 PDT
Created attachment 642934 [details] [diff] [review]
only generate error messages for simple cases

I talked with bholley about this on IRC, and he said that we shouldn't just unwrap for no good reason, so instead what I do here is check for the simple case, which is that our object is a reflector.  Otherwise, we just return "Unknown".  Basically, I turn the assertion into a dynamic check.

The existing assertion didn't fail for tearoffs, but in that case the existing code would end up casting a XPCWrappedNativeTearOff to an XPCWrappedNative, which is not okay, so I just don't bother generating a detailed name in that case.

I'm not exactly sure how this result is used, as I don't see "unknown" printed to the error console anywhere, but whatever. 

I removed the bogus comment at the start of the function (as far as I can see, this has nothing in common with either mentioned function).

The code once you get the proto should be the same.
Comment 8 Andrew McCreight [:mccr8] 2012-07-17 09:08:42 PDT
Comment on attachment 642934 [details] [diff] [review]
only generate error messages for simple cases

I'm try-running this, but I doubt it will find bugs, if there are any...
Comment 9 Bobby Holley (:bholley) (busy with Stylo) 2012-07-17 11:52:20 PDT
Comment on attachment 642934 [details] [diff] [review]
only generate error messages for simple cases

Please add a comment and check in the testcase. r=bholley with that.
Comment 10 Andrew McCreight [:mccr8] 2012-07-17 12:35:05 PDT
I have a little bit of a comment in there already.  Should I specifically mention that we aren't handling wrapped reflectors?
Comment 11 Bobby Holley (:bholley) (busy with Stylo) 2012-07-17 12:49:54 PDT
(In reply to Andrew McCreight [:mccr8] from comment #10)
> I have a little bit of a comment in there already.  Should I specifically
> mention that we aren't handling wrapped reflectors?

Yes, mention that we're specifically bouncing for security wrappers.
Comment 12 Andrew McCreight [:mccr8] 2012-07-18 13:33:44 PDT
Added the test, added a more explicit comment.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b42e44601363
Comment 13 Ed Morley [:emorley] 2012-07-19 07:40:42 PDT
https://hg.mozilla.org/mozilla-central/rev/b42e44601363
Comment 14 Daniel Veditz [:dveditz] 2012-07-19 16:51:23 PDT
Given Andrew's guess at the end of comment 5 that this is related to cpg can we safely say ESR-10 is unaffected, or do we need to test it?
Comment 15 Andrew McCreight [:mccr8] 2012-07-21 02:21:36 PDT
(In reply to Daniel Veditz [:dveditz] from comment #14)
> Given Andrew's guess at the end of comment 5 that this is related to cpg can
> we safely say ESR-10 is unaffected, or do we need to test it?

The code in this function is pretty junky, and has an assertion that is too weak.  It is probably a good idea to land my patch on ESR10.  I'd guess that this particular test case wouldn't affect it, but a variant might.
Comment 16 Andrew McCreight [:mccr8] 2012-07-21 02:25:47 PDT
Comment on attachment 642934 [details] [diff] [review]
only generate error messages for simple cases

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This code is pretty old.
If this is not a sec:{high,crit} bug, please state case for ESR consideration: I'm not sure exactly how bad this is, but it involves some incorrect casts, so it could be bad.
User impact if declined: possible security problems
Fix Landed on Version: 17
Risk to taking this patch (and alternatives if risky): low.  The patch looks big, but it really just adds a single guard. This is also modifying error message generation code, so it is probably not very common.
String or UUID changes made by this patch: none
Comment 17 Alex Keybl [:akeybl] 2012-07-23 11:24:35 PDT
Comment on attachment 642934 [details] [diff] [review]
only generate error messages for simple cases

[Triage Comment]
Low risk fix for a security issue with amorphous fallout, and we're early in the cycle. Let's get this landed on all branches.
Comment 19 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-14 15:06:04 PDT
Should this testcase be put in-testsuite?
Comment 20 Andrew McCreight [:mccr8] 2012-08-14 15:07:34 PDT
Not until 15 is released.
Comment 21 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-14 15:32:42 PDT
(In reply to Andrew McCreight [:mccr8] from comment #20)
> Not until 15 is released.

Is there a patch ready to go? If not, I'm willing to help provided some guidance.
Comment 22 Andrew McCreight [:mccr8] 2012-08-14 16:55:44 PDT
The test would just be loading the test case and seeing if anything bad happens, preferably in a debug build.  I removed the actual assertion that was tripped, so it may not be super useful.
Comment 23 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-14 16:59:57 PDT
I'll remove in-testsuite for now since this looks like it needs to be verified manually before Firefox 15 is released.
Comment 24 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-27 11:47:38 PDT
Confirmed reproducible with 2012-06-05 mozilla-central debug build.

Verified fixed with:
 * 2012-08-27 Firefox 17.0a1 Debug
 * 2012-08-27 Firefox 16.0a2 Debug
 * 2012-08-25 Firefox 15.0 Debug
 * 2012-08-25 Firefox 10.0.7esr Debug

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