1543845 - Stack overflow crash with failed Ci.nsINamed instances

Status: NEW

(Core :: XPConnect, defect, P3)

Description

[Oriol Brufau [:Oriol]]

Open the browser console and enter one of these:

Cc["@mozilla.org/profile/migrator;1?app=browser&type=firefox"].createInstance(Ci.nsINamed);

Cc["@mozilla.org/remote-web-navigation;1"].createInstance(Ci.nsINamed);

Result: stack overflow crash
https://crash-stats.mozilla.org/report/index/02ee7a91-774a-4abf-ad50-a8e9a0190411
https://crash-stats.mozilla.org/report/index/a5d8bde0-d12d-46bc-8e20-35e240190411

Regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e8ad5619116c&tochange=c999e7890f8b

I guess other classes changed in bug 1524688 are also affected, but I haven't tested them all.

Firefox ASan shows

==2348==ERROR: AddressSanitizer: stack-overflow on address 0x7ffe1022fc38 (pc 0x56278664f9f1 bp 0x7ffe10230490 sp 0x7ffe1022fc40 T0)
    #0 0x56278664f9f0 in __asan_memset /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:27:3
    #1 0x7fbacd089451 in PodAssign<mozilla::detail::HashTable<const JS::PropertyKey, mozilla::HashSet<JS::PropertyKey, mozilla::DefaultHasher<jsid>, js::TempAllocPolicy>::SetHashPolicy, js::TempAllocPolicy> > /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/PodOperations.h:78:3
    #2 0x7fbacd089451 in HashTable /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/HashTable.h:1514
    #3 0x7fbacd089451 in HashSet /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/HashTable.h:464
    #4 0x7fbacd089451 in GCHashSet /builds/worker/workspace/build/src/obj-firefox/dist/include/js/GCHashTable.h:256
    #5 0x7fbacd089451 in DispatchWrapper<JS::GCHashSet<JS::PropertyKey, mozilla::DefaultHasher<jsid>, js::TempAllocPolicy> > /builds/worker/workspace/build/src/obj-firefox/dist/include/js/RootingAPI.h:840
    #6 0x7fbacd089451 in Rooted<JSContext *, JS::GCHashSet<JS::PropertyKey, mozilla::DefaultHasher<jsid>, js::TempAllocPolicy> > /builds/worker/workspace/build/src/obj-firefox/dist/include/js/RootingAPI.h:1025
    #7 0x7fbacd089451 in Snapshot(JSContext*, JS::Handle<JSObject*>, unsigned int, JS::MutableHandle<JS::StackGCVector<JS::PropertyKey, js::TempAllocPolicy> >) /builds/worker/workspace/build/src/js/src/vm/Iteration.cpp:444
    #8 0x7fbacd83e0d7 in JS_Enumerate(JSContext*, JS::Handle<JSObject*>, JS::MutableHandle<JS::GCVector<JS::PropertyKey, 0ul, js::TempAllocPolicy> >) /builds/worker/workspace/build/src/js/src/jsapi.cpp:2530:8
    #9 0x7fbac256099d in (anonymous namespace)::GetFunctionName(JSContext*, JS::Handle<JSObject*>) /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedJSClass.cpp:250:10
    #10 0x7fbac2560c69 in (anonymous namespace)::GetFunctionName(JSContext*, JS::Handle<JSObject*>) /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedJSClass.cpp:271:12
    #11 0x7fbac2560c69 in (anonymous namespace)::GetFunctionName(JSContext*, JS::Handle<JSObject*>) /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedJSClass.cpp:271:12
    #12 0x7fbac2560c69 in (anonymous namespace)::GetFunctionName(JSContext*, JS::Handle<JSObject*>) /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedJSClass.cpp:271:12
    #13 0x7fbac2560c69 in (anonymous namespace)::GetFunctionName(JSContext*, JS::Handle<JSObject*>) /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedJSClass.cpp:271:12
    #14 0x7fbac2560c69 in (anonymous namespace)::GetFunctionName(JSContext*, JS::Handle<JSObject*>) /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedJSClass.cpp:271:12
    #15 0x7fbac2560c69 in (anonymous namespace)::GetFunctionName(JSContext*, JS::Handle<JSObject*>) /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedJSClass.cpp:271:12
    #16 0x7fbac2560c69 in (anonymous namespace)::GetFunctionName(JSContext*, JS::Handle<JSObject*>) /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedJSClass.cpp:271:12

and GetFunctionName keeps calling itself again and again.
https://searchfox.org/mozilla-central/rev/6dab6dad9cc852011a14275a8b2c2c03ed7600a7/js/xpconnect/src/XPCWrappedJSClass.cpp#239,271

Comment 1

[Pascal Chevrel:pascalc]

Almost no crashes on 66, 67 -> wontfix

Comment 2

[Oriol Brufau [:Oriol]]

66 is unaffected, the code just throws an error instead of crashing, as expected.

Comment 3

[Andrew McCreight [:mccr8]]

I looked at this a little bit. This code takes an object, unwraps it, then looks for a single property that is an object. If it has one, it recursively calls GetFunctionName. In this case, the property is wrappedJSObject, which ends up being the exact same object, so we get the infinite loop. I don't know how Kris's changes caused this to break. A trivial fix would be to just detect the loop and give up in that case.

Comment 4

[Andrew McCreight [:mccr8]]

If you have an old build around where this works handy, what properties does the object returned by, say,
Cc["@mozilla.org/profile/migrator;1?app=browser&type=firefox"].createInstance(Ci.nsISupports)
have? Maybe the old way of creating things just happened to have another property on the object, so this code would ignore it.

Comment 5

[Kris Maglione [:kmag]]

I guess the difference is that the old code had a JS-implemented factory, so the QueryInterface went straight to the raw JS object rather than going through XPConnect, and that threw because the objects don't explicitly implement nsINamed. That's arguably a bug. If you'd done .getService().QueryInterface(Ci.nsINamed) you'd get the same behavior you get with the pure gerService call now.

I suppose we should fix this, although since bug 1528383 the wrappedJSObject properties can just be removed.

Comment 6

[Oriol Brufau [:Oriol]]

Tested in 66.0.2

 Cc["@mozilla.org/profile/migrator;1?app=browser&type=firefox"].createInstance(Ci.nsISupports)

works and produces a XPCWrappedNative_NoHelper object with a QueryInterface method, like in Nightly.

Cc["@mozilla.org/profile/migrator;1?app=browser&type=firefox"].createInstance(Ci.nsINamed);

reports a NS_NOINTERFACE: error coming from XPCOMUtils.jsm:433, and throws this exception:

Exception
​  columnNumber: 0
​  data: null
​  filename: "debugger eval code"
​  lineNumber: 1
​  location: XPCWrappedNative_NoHelper { QueryInterface: QueryInterface(), filename: Getter, name: Getter, … }
​  message: "ComponentManager::CreateInstance returned failure code:"
​  name: "NS_ERROR_XPC_CI_RETURNED_FAILURE"
​  result: 2153185301
​  stack: "@debugger eval code:1:7\ngetEvalResult@resource://devtools/server/actors/webconsole/eval-with-debugger.js:134:18\nexports.evalWithDebugger@resource://devtools/server/actors/webconsole/eval-with-debugger.js:105:18\nevaluateJS@resource://devtools/server/actors/webconsole.js:1005:22\nevaluateJSAsync@resource://devtools/server/actors/webconsole.js:910:22\nonPacket@resource://devtools/server/main.js:1286:15\nsend/<@resource://devtools/shared/transport/local-transport.js:64:11\nexports.makeInfallible/<@resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14\nexports.makeInfallible/<@resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14\n"
​  <prototype>: ExceptionPrototype { toString: toString(), name: Getter, message: Getter, … }

Comment 7

[Oriol Brufau [:Oriol]]

(In reply to Kris Maglione [:kmag] from comment #5)
You are right, Cc["@mozilla.org/profile/migrator;1?app=browser&type=firefox"].getService().QueryInterface(Ci.nsINamed) already crashed before bug 1524688. The regression window for that is https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=df9beb781895fcd0493c21e95ad313e0044515ec&tochange=564e82f0f289af976da01c2d50507017bbc152b5

Comment 8

[Andrew McCreight [:mccr8]]

That includes the patch that implemented this nsIName behavior, so that makes sense.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:neha, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 10

[Patricia Lawless]

Bulk change of P3 carryover bugs to wontfix for 68.