940779 - Restrict the scope of a MessageEventInit so it cannot be live across a GC call

Status: RESOLVED FIXED

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

Description

[Steve Fink [:sfink] [:s:]]

Hazard:

Function 'uint8 mozilla::dom::workers::WorkerPrivate::ConnectMessagePort(JSContext*, uint64)' has unrooted 'init' of type 'mozilla::dom::MessageEventInit' live across GC call 'nsRefPtr<T>::~nsRefPtr() [with T = mozilla::dom::MessagePortList]' at dom/workers/WorkerPrivate.cpp:5288

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Restrict the scope of a MessageEventInit so it cannot be live across a GC call

This one is *probably* a false positive. MessageEventInit has an Optional<JS::Value>, which has a destructor. That's enough for the analysis to conclude that ~MessageEventInit touches its members, and so should be considered live. The nsRefPtr gets destructed first, and it can conceivably GC, so we have a gcpointer held live across a GC.

In reality, I doubt anything the destructor does depends on the exact pointer values of those fields, but that's a lot of handwaving. Better to remove the possibility.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8334973 [details] [diff] [review]
Restrict the scope of a MessageEventInit so it cannot be live across a GC call

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

::: dom/workers/WorkerPrivate.cpp
@@ +5259,5 @@
>      return false;
>    }
>  
> +  nsRefPtr<nsDOMMessageEvent> event;
> +  {

Nit: Add a comment about why you're bracing or someone is likely to undo your hard work ;)

@@ +5267,5 @@
> +    init.mSource = &jsPort.toObject();
> +
> +    ErrorResult rv;
> +    event = nsDOMMessageEvent::Constructor(globalObject, aCx,
> +                                           NS_LITERAL_STRING("connect"), init, rv);

Nit: Can you wrap rv to the next line so that we stay under 80 chars?

Comment 3

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/4088133a80e6

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4088133a80e6

Comment 5

[Steve Fink [:sfink] [:s:]]

Attachment: MessageEventInit is a dictionary

I am going to back out the previous patch and apply this instead. Much better.

Comment 6

[Steve Fink [:sfink] [:s:]]

backout:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b9809c5b248c
new patch:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2eb23296f0d1

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2eb23296f0d1