744772 - xhr js::BaseShape::getObjectFlags crash

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Patrick McManus [:mcmanus]]

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]]

e636439e342f was the tip cset of my build.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I went through the slideshow twice without hitting this.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

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]]

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]]

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 [:bzbarsky]]

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]]

Attachment: 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 [:bzbarsky]]

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]]

Attachment: 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 [:bzbarsky]]

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]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/17b552b979d5

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/17b552b979d5

Comment 14

[Peter Van der Beken [:peterv]]

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]]

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]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7d702f45c71a

Comment 17

[Daniel Veditz [:dveditz]]

Since this was never in a shipping build (not even beta) we don't need to keep this hidden.