Closed Bug 828502 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: XPCOM, defect)

ARM
Gonk (Firefox OS)
defect
Not set
blocker

Tracking

()

RESOLVED FIXED
mozilla21
blocking-basecamp +
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed

People

(Reporter: mikeh, Assigned: mikeh)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch fix (obsolete) — Splinter Review
The attached patch fixes this problem, and adds an NS_WARNING to the otherwise-blind MOZ_CRASH() call.
Attachment #699905 - Flags: review?(bobbyholley+bmo)
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.
Attachment #699905 - Flags: review?(bobbyholley+bmo) → review+
Addressing feedback from review, carrying r+ forward.
Attachment #699905 - Attachment is obsolete: true
Attachment #700045 - Flags: review+
This blocks a blocker so blocking.
blocking-basecamp: --- → +
https://hg.mozilla.org/mozilla-central/rev/ac6c200b012d
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: