828502 - nsMainThreadPtrHandle can be initialized with a nullptr, and ::get() will crash on it

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: fix

The attached patch fixes this problem, and adds an NS_WARNING to the otherwise-blind MOZ_CRASH() call.

Comment 1

[Bobby Holley (:bholley)]

Comment on attachment 699905 [details] [diff] [review]
fix

Review of attachment 699905 [details] [diff] [review]:
-----------------------------------------------------------------

r=bholley with comments addressed.

::: xpcom/glue/nsProxyRelease.h
@@ +129,5 @@
>  
>    T* get() {
>      // Nobody should be touching the raw pointer off-main-thread.
> +    if (MOZ_UNLIKELY(!NS_IsMainThread())) {
> +      NS_WARNING("Attempt to get() raw from nsMainThreadPtrHolder off main thread!");

Make this "Can't dereference nsMainThreadPtrHolder off main thread".

@@ +173,5 @@
>    operator nsMainThreadPtrHolder<T>*() { return mPtr.get(); }
>  
>    // These all call through to nsMainThreadPtrHolder, and thus implicitly
>    // assert that we're on the main thread. Off-main-thread consumers must treat
>    // these handles as opaque.

Now that get() is no longer guaranteed to go through nsMainThreadPtrHolder, we no longer get deterministic crashes for off-main-thread usage, so people may not notice the problem if their pointers happen to be null. At the same time, another release-mode check is hardly ideal. So how about adding a MOZ_ASSERT that we're on the main thread in this function?

@@ +176,5 @@
>    // assert that we're on the main thread. Off-main-thread consumers must treat
>    // these handles as opaque.
> +  T* get()
> +  {
> +    if (mPtr.get()) {

Again, no need to .get() COMPtrs/RefPtrs for most purposes, including boolean tests.

Comment 2

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: fix (for landing), v2

Addressing feedback from review, carrying r+ forward.

Comment 3

[Johnny Stenback (:jst)]

This blocks a blocker so blocking.

Comment 4

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac6c200b012d

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ac6c200b012d

Comment 6

[Mike Habicher [:mikeh] (high bugzilla latency)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/65be9e98368a

Comment 7

[Mike Habicher [:mikeh] (high bugzilla latency)]

Back out due to b2g18 bustage: https://hg.mozilla.org/releases/mozilla-b2g18/rev/47f713c2fb4c

Comment 8

[Mike Habicher [:mikeh] (high bugzilla latency)]

(Once more, with feeling!)
https://hg.mozilla.org/releases/mozilla-b2g18/rev/dcc9202c0fff