932466 - Fix two trivial exact rooting hazards in dom

Status: RESOLVED FIXED

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

Description

[Terrence Cole [:terrence]]

Attachment: hazards_misc_browser-v0.diff

I'm not entirely sure how dom::RootedDictionary is supposed to GC, but it's simple enough to just swap the declaration order.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> I'm not entirely sure how dom::RootedDictionary is supposed to GC

It can't, but the analysis can't figure that out.

What it sees is that RootedDictionary<MediaTrackConstraints>::RootedDictionary calls MediaTrackConstraints::MediaTrackConstraints, which calls MediaTrackConstraints::Init(nullptr, JS::NullHandleValue).

Now if you look at Init(), it has:

  // Passing a null JSContext is OK only if we're initing from null,
  // Since in that case we will not have to do any property gets
  MOZ_ASSERT_IF(!cx, val.isNull());
...
  bool isNull = val.isNullOrUndefined();

and then a bunch of code like this:

  if (!isNull && !JS_GetPropertyById(cx, &val.toObject(), atomsCache->mandatory_id, &temp.ref())) {
    return false;
  }

which can clearly gc... but not when called with val as a NullValue.  So it's a false positive, but a somewhat non-obvious one.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 824227 [details] [diff] [review]
hazards_misc_browser-v0.diff


> 
>-  JS::Value result;
>+  JS::RootedValue result(aCx);
JS::Rooted<JS::Value> for now

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

...and I think it is ok to just move the thing to be before dom::RootedDictionary

Comment 4

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/93f195bf9008

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/93f195bf9008