Closed Bug 1453951 Opened 2 years ago Closed 2 years ago
Crash in non-virtual thunk to ns
In Process Tab Child Global::Wrap Object
This bug was filed from the Socorro interface and is report bp-0c771d0c-c85f-4157-b06e-c2dc90180413. ============================================================= Seen while looking at nightly crashes - small volume Mac crash with Crash Reason MOZ_CRASH(We should never get here!): https://bit.ly/2EJ42VR. Crashes appear to have started with Build 20180412001050. Perhaps :peterv has some insight since he has worked in this area. Top 10 frames of crashing thread: 0 XUL non-virtual thunk to nsInProcessTabChildGlobal::WrapObject dom/base/nsInProcessTabChildGlobal.h:53 1 XUL mozilla::dom::NativeInterface2JSObjectAndThrowIfFailed dom/bindings/BindingUtils.cpp:1038 2 XUL mozilla::dom::WrapNativeHelper<nsISupports, false>::Wrap dom/bindings/BindingUtils.h:1664 3 XUL mozilla::dom::ChildSHistoryBinding::Wrap dom/bindings/BindingUtils.h:1757 4 XUL non-virtual thunk to mozilla::dom::ChildSHistory::WrapObject dist/include/mozilla/dom/ChildSHistoryBinding.h:45 5 XUL XPCConvert::NativeInterface2JSObject js/xpconnect/src/XPCConvert.cpp:757 6 XUL XPCConvert::NativeData2JS js/xpconnect/src/XPCConvert.cpp:344 7 XUL XPCWrappedNative::CallMethod js/xpconnect/src/XPCWrappedNative.cpp:1484 8 XUL XPC_WN_GetterSetter js/xpconnect/src/xpcprivate.h:1610 9 XUL js::InternalCallOrConstruct js/src/vm/JSContext-inl.h:290 =============================================================
Hmm, I don't think we wrap nsInProcessTabChildGlobal correctly.
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Priority: -- → P2
Seems to be a startup crash!
Comment on attachment 8974132 [details] [diff] [review] v1 nsInProcessTabChildGlobal::Init always returns NS_OK. Why not just do it as part of the constructor? That does involve handing out a partially-constructed this-pointer, but that seems OK in this particular case.... especially if you do it at the end of the ctor and mark the nsInProcessTabChildGlobal class final. At least as long as callees don't start refcounting it. Alternately, to be really safe, you could make both the ctor and Init() private and have a Create() method that does the ctor, takes a ref, calls init, returns already_AddRefed. In either case seems like you would not need the mInitialized member anymore, right?
Removed InitTabChildGlobal, also kept just the things that can fail into Init.
Comment on attachment 8974493 [details] [diff] [review] v2 r=me
Attachment #8974493 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/635bea9e749d77888d28f422c7ef4b1862e078c4 Bug 1453951 - Crash in non-virtual thunk to nsInProcessTabChildGlobal::WrapObject. r=bz.
Crash Signature: [@ non-virtual thunk to nsInProcessTabChildGlobal::WrapObject] → [@ non-virtual thunk to nsInProcessTabChildGlobal::WrapObject] [@ nsInProcessTabChildGlobal::WrapObject ]
Looks like the fix worked. Please request Beta approval on this as soon as possible so we can hopefully get it into Monday's 61.0b5 build.
Comment on attachment 8974493 [details] [diff] [review] v2 Approval Request Comment [Feature/Bug causing the regression]: bug 888600 [User impact if declined]: crash [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: calls code to init the object earlier, so we don't crash because of uninited state [String changes made/needed]: none
Attachment #8974493 - Flags: approval-mozilla-beta?
Comment on attachment 8974493 [details] [diff] [review] v2 Nightly crash data confirms that this patch was effective. Approved for 61.0b5.
Attachment #8974493 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.