906901 - Use webIDL dictionaries for BrowerElementParent's event details

Status: RESOLVED FIXED

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

Comment 1

[David Zbarsky (:dzbarsky)]

Attachment: Use webIDL dictionaries for BrowerElementParent's event details

Comment 2

[David Zbarsky (:dzbarsky)]

Attachment: Use webIDL dictionaries for BrowerElementParent's event details r=bz

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 793057 [details] [diff] [review]
Use webIDL dictionaries for BrowerElementParent's event details r=bz

> DispatchCustomDOMEvent

Please just pass a cx to this method. All the callers have one, and it would make sure the cx and aDetailValue are same-compartment.

>+  return NS_SUCCEEDED(nsEventDispatcher::DispatchDOMEvent(aFrameElement, 

I'd prefer we keep the nsresult temporary here.

>+  JS::Rooted<JSObject*> global(cx, aPopupFrameElement->OwnerDoc()->GetScopeObject()->GetGlobalJSObject());

This might need some null-checks.

>+  detail.ToObject(cx, global, &val);

If that throws, don't you need to actually report the exception and such?

>+  detail.ToObject(cx, JS::NullPtr(), &val);

Why is this not trying to get a useful global?

Comment 4

[David Zbarsky (:dzbarsky)]

(In reply to Boris Zbarsky [:bz] from comment #3)
> Comment on attachment 793057 [details] [diff] [review]

> 
> >+  detail.ToObject(cx, global, &val);
> 
> If that throws, don't you need to actually report the exception and such?
> 

I can return false in that case.  I'm not sure if that will actually report an exception, but it seems like ToObject only fails on OOM.

> >+  detail.ToObject(cx, JS::NullPtr(), &val);
> 
> Why is this not trying to get a useful global?

We need the global for wrapping the node.  It's ok to have a null global when the dictionary only contains ints.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> I'm not sure if that will actually report an exception,

Checking the callers, one seems to be ProvideWidnow, which is called from C++ code in some cases, hence will not report.  <sigh>

> but it seems like ToObject only fails on OOM.

With bug 862848 that might be true, yeah.  OK, let's just return false for now?  Or runtime-abort if we fail?

> We need the global for wrapping the node.

I see.  Worth a comment at the site passing null, in case someone adds more members to the dictionary.

Comment 6

[David Zbarsky (:dzbarsky)]

Attachment: Use webIDL dictionaries for BrowerElementParent's event details r=bz

Comment 7

[David Zbarsky (:dzbarsky)]

Attachment: interdiff

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 794006 [details] [diff] [review]
interdiff

aCx for the argument, please.

The only remaining worry I have is what compartment the val ends up with for the mozbrowserasyncscroll bit.  If that's only relevant to chrome, it probably doesn't matter...

r=me if that event is chrome-only.

Comment 9

[David Zbarsky (:dzbarsky)]

(In reply to Boris Zbarsky [:bz] from comment #8)
> Comment on attachment 794006 [details] [diff] [review]
> interdiff
> 
> aCx for the argument, please.
> 
> The only remaining worry I have is what compartment the val ends up with for
> the mozbrowserasyncscroll bit.  If that's only relevant to chrome, it
> probably doesn't matter...
> 
> r=me if that event is chrome-only.

Yeah, it's only used in the BEP jsm.

Comment 10

[David Zbarsky (:dzbarsky)]

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

Comment 11

[David Zbarsky (:dzbarsky)]

And https://hg.mozilla.org/integration/mozilla-inbound/rev/27eb88629c56 for the renaming I forgot to do.

Comment 12

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/26944521805d for Windows opt test crashes, that may well turn out to be needs-CLOBBER.

Comment 13

[Gary [:streetwolf52]]

Tried the cset with this patch on Windows 8 and it caused about:support to crash.

Good - http://hg.mozilla.org/integration/mozilla-inbound/rev/5afcb60af43b

Bad - http://hg.mozilla.org/integration/mozilla-inbound/rev/a26582e3c44d

Comment 14

[David Zbarsky (:dzbarsky)]

Do you have a stack?

Comment 15

[David Zbarsky (:dzbarsky)]

relanded with a clobber: https://hg.mozilla.org/integration/mozilla-inbound/rev/bedb4f9707ce

Comment 16

[Phil Ringnalda (:philor)]

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

Comment 17

[Gary [:streetwolf52]]

No more crashes.