Open Bug 1543845 Opened 6 years ago Updated 2 years ago

Stack overflow crash with failed Ci.nsINamed instances

Categories

(Core :: XPConnect, defect, P3)

67 Branch
defect

Tracking

()

Tracking Status
firefox-esr60 --- unaffected
firefox66 --- unaffected
firefox67 --- wontfix
firefox68 --- wontfix

People

(Reporter: Oriol, Unassigned)

References

(Regression)

Details

(Keywords: crash, regression)

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

Component: XPCOM → XPConnect
Component: XPConnect → XPCOM
Flags: needinfo?(kmaglione+bmo)

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

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.

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.

Flags: needinfo?(oriol-bugzilla)

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.

Flags: needinfo?(kmaglione+bmo)
No longer regressed by: 1524688
Component: XPCOM → XPConnect

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, … }
Flags: needinfo?(oriol-bugzilla)
Crash Signature: [@ Snapshot ]

(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

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

Regressed by: 1382172

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(nkochar)
Flags: needinfo?(nkochar)
Priority: -- → P3

Bulk change of P3 carryover bugs to wontfix for 68.

Has Regression Range: --- → yes
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.