Last Comment Bug 638026 - "ASSERTION: should have a JS object by this point" with setUserData, GC
: "ASSERTION: should have a JS object by this point" with setUserData, GC
Status: RESOLVED FIXED
[sg:nse] fixed-in-tracemonkey
: assertion, testcase
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap) (please use needinfo!)
:
Mentors:
Depends on:
Blocks: 326633 594645 637226
  Show dependency treegraph
 
Reported: 2011-03-01 23:49 PST by Jesse Ruderman
Modified: 2011-04-14 11:38 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
testcase (requires extension for GC) (556 bytes, text/html)
2011-03-01 23:49 PST, Jesse Ruderman
no flags Details
stack traces (3.60 KB, text/plain)
2011-03-01 23:50 PST, Jesse Ruderman
no flags Details
Proposed fix (1.12 KB, patch)
2011-03-07 14:17 PST, Blake Kaplan (:mrbkap) (please use needinfo!)
jst: review+
Details | Diff | Review

Description Jesse Ruderman 2011-03-01 23:49:42 PST
Created attachment 516190 [details]
testcase (requires extension for GC)

1. Install 'DOM Fuzz Lite' from
    https://www.squarefree.com/extensions/domFuzzLite.xpi
2. Load the testcase.

###!!! ASSERTION: should have a JS object by this point: 'win->GetOuterWindowInternal()->IsCreatingInnerWindow()', file dom/base/nsDOMClassInfo.cpp, line 5022

###!!! ASSERTION: Non-global object has the wrong flags: '!(jsclazz->flags & JSCLASS_IS_GLOBAL)', file js/src/xpconnect/src/xpcwrappednative.cpp, line 1153

Security-sensitive because I'm scared of anything that involves GC. Or inner and outer windows.
Comment 1 Jesse Ruderman 2011-03-01 23:50:06 PST
Created attachment 516191 [details]
stack traces
Comment 2 Andreas Gal :gal 2011-03-01 23:59:42 PST
This doesn't look exploitable. Why blocking?
Comment 3 Andreas Gal :gal 2011-03-02 11:39:41 PST
b- please renom if we see any indication that this is indeed exploitable
Comment 4 Johnny Stenback (:jst, jst@mozilla.com) 2011-03-03 13:22:45 PST
Given that we don't know of this being exploitable or anything, marking this sg:nse, and keeping it closed for now. Blake, can you have a look?
Comment 5 Blake Kaplan (:mrbkap) (please use needinfo!) 2011-03-07 14:17:57 PST
Created attachment 517543 [details] [diff] [review]
Proposed fix

So, the problem here is that the variant holds a reference to the outer window proxy and a reference to the current inner window's C++ object. After the iframe navigates, we brain transplant the outer window, and the old inner window's JS object has no more references to it, so we collect it. Later, we attempt to wrap the inner window into JS and recreate its JS object (and hit the assertion). This patch fixes this by making us hold a reference to the non-brain-transplanted inner object, meaning we won't recreate the JS object. This means we'll hold the old inner window alive longer, but I'm OK with that.
Comment 6 Blake Kaplan (:mrbkap) (please use needinfo!) 2011-03-08 15:23:42 PST
http://hg.mozilla.org/tracemonkey/rev/41ea0740c97d
Comment 7 Daniel Veditz [:dveditz] 2011-03-10 13:14:03 PST
Blake: we don't have brain transplants on the 1.9.x branches, but we do have inner and outer windows and we have seen the assertions in comment 0 (bug 637226). Do we need a variant of this fix on branches? Maybe not if it's truly unexploitable.
Comment 8 Blake Kaplan (:mrbkap) (please use needinfo!) 2011-03-10 14:40:10 PST
No. On older branches, the variant ends up holding a strong reference to the outer window, not the inner and the jsval that it holds is also for the outer, so it won't be garbage collected. Because the value and the nsISupports match there is no bug.
Comment 9 Blake Kaplan (:mrbkap) (please use needinfo!) 2011-04-14 11:38:08 PDT
http://hg.mozilla.org/mozilla-central/rev/41ea0740c97d

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