Last Comment Bug 744772 - xhr js::BaseShape::getObjectFlags crash
: xhr js::BaseShape::getObjectFlags crash
Status: RESOLVED FIXED
[sg:critical][advisory-tracking-][qa-]
: crash, regression, sec-critical
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 14 Branch
: All All
: -- critical (vote)
: mozilla15
Assigned To: Peter Van der Beken [:peterv]
:
:
Mentors:
Depends on: 765034
Blocks: 740069
  Show dependency treegraph
 
Reported: 2012-04-12 07:12 PDT by Patrick McManus [:mcmanus]
Modified: 2012-08-14 08:16 PDT (History)
15 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
fixed
fixed
unaffected


Attachments
v1 (4.20 KB, patch)
2012-04-27 08:02 PDT, Peter Van der Beken [:peterv]
bzbarsky: review+
Details | Diff | Splinter Review
v1.1 (5.01 KB, patch)
2012-05-02 06:08 PDT, Peter Van der Beken [:peterv]
bzbarsky: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Patrick McManus [:mcmanus] 2012-04-12 07:12:32 PDT
Using a locally built x64 tree from m-c on 04-11 I get crashes fairly regularly at different points in this slideshow (just going through it a time or two is generally enough):

http://www.bankrate.com/finance/personal-finance/high-paying-college-majors-1.aspx

the crashes look to my unexperienced eye as a dom binding issue with XHR, but I didn't try and really narrow it down.

(gdb) bt
#0  0x00007ffff545e9be in js::BaseShape::getObjectFlags (this=0xdadadadadadadada) at ../../../pipemq/js/src/jsscope.h:352
#1  0x00007ffff54b40be in js::Shape::getObjectFlags (this=0x7fffd993d420) at ../../../pipemq/js/src/jsscope.h:609
#2  0x00007ffff5512e69 in JSObject::setFlag (this=0x7fffd993d200, cx=0x7fffcfe5a400, flag_=4096, generateShape=JSObject::GENERATE_NONE) at ../../../pipemq/js/src/jsscope.cpp:1112
#3  0x00007ffff547aab6 in JSObject::setNewTypeUnknown (this=0x7fffd993d200, cx=0x7fffcfe5a400) at ../../../pipemq/js/src/jsinfer.cpp:5603
#4  0x00007ffff53d8f17 in JS_NewObject (cx=0x7fffcfe5a400, jsclasp=0x7ffff677a160, proto=0x7fffd993d200, parent=0x7fffc9555880) at ../../../pipemq/js/src/jsapi.cpp:3285
#5  0x00007ffff4e3807f in mozilla::dom::bindings::CreateInterfacePrototypeObject (cx=0x7fffcfe5a400, global=0x7fffc9555880, parentProto=0x7fffd993d200, protoClass=0x7ffff677a160, methods=0x0,
    properties=0x7ffff677a320, constants=0x0) at ../../../pipemq/dom/bindings/Utils.cpp:72
#6  0x00007ffff4e3823e in mozilla::dom::bindings::CreateInterfaceObjects (cx=0x7fffcfe5a400, global=0x7fffc9555880, parentProto=0x7fffd993d200, protoClass=0x7ffff677a160, constructorClass=0x0,
    methods=0x0, properties=0x7ffff677a320, constants=0x0, staticMethods=0x0, name=0x0) at ../../../pipemq/dom/bindings/Utils.cpp:110
#7  0x00007ffff4e3556a in mozilla::dom::bindings::prototypes::XMLHttpRequestEventTarget::CreateInterfaceObjects (aCx=0x7fffcfe5a400, aGlobal=0x7fffc9555880)
    at XMLHttpRequestEventTargetBinding.cpp:482
#8  0x00007ffff4e2b137 in mozilla::dom::bindings::prototypes::XMLHttpRequestEventTarget::GetProtoObject (aCx=0x7fffcfe5a400, aGlobal=0x7fffc9555880)
    at ../../dist/include/mozilla/dom/bindings/XMLHttpRequestEventTargetBinding.h:73
#9  0x00007ffff4e2ec7a in mozilla::dom::bindings::prototypes::XMLHttpRequest::CreateInterfaceObjects (aCx=0x7fffcfe5a400, aGlobal=0x7fffc9555880) at XMLHttpRequestBinding.cpp:1282
#10 0x00007ffff4e2b0ab in mozilla::dom::bindings::prototypes::XMLHttpRequest::GetProtoObject (aCx=0x7fffcfe5a400, aGlobal=0x7fffc9555880)
    at ../../dist/include/mozilla/dom/bindings/XMLHttpRequestBinding.h:91
#11 0x00007ffff4e2f0fe in mozilla::dom::bindings::prototypes::XMLHttpRequest::DefineDOMInterface (aCx=0x7fffcfe5a400, aScope=0x7fffccbd6df0, aEnabled=0x7fffffff771e)
    at XMLHttpRequestBinding.cpp:1361
Comment 1 Patrick McManus [:mcmanus] 2012-04-12 07:13:33 PDT
e636439e342f was the tip cset of my build.
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-12 08:56:23 PDT
I went through the slideshow twice without hitting this.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2012-04-12 10:31:51 PDT
OK.  Just reproduced this crash, in a 32-bit debug build.

The lastProperty() of |this| in JSObject::setFlag is 0xdadadada.

The object in question is the proto we passed to JS_NewObject.  In fact, all its fields are 0xdadadada, which I assume means it got GCed.

Peter, that object came out of EventTarget::GetProtoObject.  Are we tracing the proto object array properly?
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2012-04-12 10:40:37 PDT
I tried to reproduce this with a testcase like so:

  window.XMLHttpRequest.prototype = null;
  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");  
  Components.utils.forceGC();
  var xhr = new XMLHttpRequest();

but I don't get a crash there...
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2012-04-12 15:12:03 PDT
Dead pointer, bad mojo.  Presumably this hasn't landed anywhere too awful yet, but as far as I know we customarily hide even temporarily-present security bugs (but we unhide them that much faster after they're fixed, as partial compensation).
Comment 6 David Mandelin [:dmandelin] 2012-04-26 18:58:16 PDT
Comment 3 has the right question--this is a regression from bug 740069 caused by the proto array stored in DOM_PROTOTYPE_SLOT not being traced. This is TraceXPCGlobal:

void
TraceXPCGlobal(JSTracer *trc, JSObject *obj)
{
#ifdef DEBUG
    if (trc->callback == VerifyTraceXPCGlobalCalled) {
        // We don't do anything here, we only want to verify that TraceXPCGlobal
        // was called.
        reinterpret_cast<VerifyTraceXPCGlobalCalledTracer*>(trc)->ok = true;
        return;
    }
#endif

    if (XPCWrappedNativeScope *scope = XPCWrappedNativeScope::GetNativeScope(obj))
        scope->TraceDOMPrototypes(trc);
}

It wasn't changed by bug 740069 (or anything after that, either).
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2012-04-26 20:09:06 PDT
What I don't understand is why my testcase from comment 4 didn't crash.  Hard to test a fix if I can't reproduce the crash reliably.  :(
Comment 8 Peter Van der Beken [:peterv] 2012-04-27 08:02:58 PDT
Created attachment 619034 [details] [diff] [review]
v1

(In reply to Boris Zbarsky (:bz) from comment #7)
>   window.XMLHttpRequest.prototype = null;


.prototype is { [[Writable]]: false, [[Enumerable]]: false, [[Configurable]]: false }. This crashes for me:

  delete window.XMLHttpRequestUpload;
  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");  
  Components.utils.forceGC();
  new XMLHttpRequest().upload;
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2012-04-27 08:38:31 PDT
Comment on attachment 619034 [details] [diff] [review]
v1

r=me, though should we do a chrome test to avoid enablePrivilege?
Comment 10 Peter Van der Beken [:peterv] 2012-05-02 06:08:19 PDT
Created attachment 620275 [details] [diff] [review]
v1.1

With a mochitest.

Turns out there was also a bug in the worker scope's trace, we need to move the trace call inside the check for a real wrapper, so we don't run it for a DedicatedWorkerGlobalScope's prototype object (which uses the same JSClass because worker code calls JS_InitClass).
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2012-05-02 11:37:36 PDT
Comment on attachment 620275 [details] [diff] [review]
v1.1

r=me

We should ban use of JS_InitClass in DOM code.  It never does the right thing.  :(
Comment 12 Peter Van der Beken [:peterv] 2012-05-03 06:56:16 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/17b552b979d5
Comment 13 Ed Morley [:emorley] 2012-05-04 04:01:34 PDT
https://hg.mozilla.org/mozilla-central/rev/17b552b979d5
Comment 14 Peter Van der Beken [:peterv] 2012-05-04 04:23:56 PDT
Comment on attachment 620275 [details] [diff] [review]
v1.1

[Approval Request Comment]
Regression caused by (bug #): bug 740069
User impact if declined: crashes
Testing completed (on m-c, etc.): landed on m-c (and has a crashtest)
Risk to taking this patch (and alternatives if risky): low risk, we just started tracing JS objects so that we don't end up with stale pointers to them
String changes made by this patch: None
Comment 15 Alex Keybl [:akeybl] 2012-05-06 19:47:26 PDT
Comment on attachment 620275 [details] [diff] [review]
v1.1

[Triage Comment]
Low risk crash fix that may become worse after release based upon browsing habits. Approving for Aurora 14.
Comment 16 Peter Van der Beken [:peterv] 2012-05-10 05:12:38 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/7d702f45c71a
Comment 17 Daniel Veditz [:dveditz] 2012-05-17 16:48:27 PDT
Since this was never in a shipping build (not even beta) we don't need to keep this hidden.

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