Closed Bug 1453951 Opened 2 years ago Closed 2 years ago

Crash in non-virtual thunk to nsInProcessTabChildGlobal::WrapObject


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




Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox59 --- unaffected
firefox60 --- unaffected
firefox61 + fixed
firefox62 + verified


(Reporter: marcia, Assigned: peterv)



(Keywords: crash, regression)

Crash Data


(1 file, 1 obsolete file)

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!): 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

Flags: needinfo?(peterv)
Hmm, I don't think we wrap nsInProcessTabChildGlobal correctly.
Assignee: nobody → peterv
Flags: needinfo?(peterv)
Priority: -- → P2
Seems to be a startup crash!
Attached patch v1 (obsolete) — Splinter Review
Attachment #8974132 - Flags: review?(bzbarsky)
Comment on attachment 8974132 [details] [diff] [review]

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?
Attachment #8974132 - Flags: review?(bzbarsky)
Attached patch v2Splinter Review
Removed InitTabChildGlobal, also kept just the things that can fail into Init.
Attachment #8974132 - Attachment is obsolete: true
Attachment #8974493 - Flags: review?(bzbarsky)
Comment on attachment 8974493 [details] [diff] [review]

Attachment #8974493 - Flags: review?(bzbarsky) → review+
Bug 1453951 - Crash in non-virtual thunk to nsInProcessTabChildGlobal::WrapObject. r=bz.
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Duplicate of this bug: 1461078
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.
Flags: needinfo?(peterv)
Comment on attachment 8974493 [details] [diff] [review]

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
Flags: needinfo?(peterv)
Attachment #8974493 - Flags: approval-mozilla-beta?
Comment on attachment 8974493 [details] [diff] [review]

Nightly crash data confirms that this patch was effective. Approved for 61.0b5.
Attachment #8974493 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.