Closed Bug 906901 Opened 7 years ago Closed 7 years ago

Use webIDL dictionaries for BrowerElementParent's event details

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: dzbarsky, Assigned: dzbarsky)

References

Details

Attachments

(3 files, 1 obsolete file)

No description provided.
Attachment #793057 - Flags: review?(bzbarsky)
Attachment #792443 - Attachment is obsolete: true
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?
Attachment #793057 - Flags: review?(bzbarsky) → review-
(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.
> 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.
Attached patch interdiffSplinter Review
Assignee: nobody → dzbarsky
Attachment #794006 - Flags: review?(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.
Attachment #794006 - Flags: review?(bzbarsky) → review+
(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.
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.
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
Do you have a stack?
Flags: needinfo?(garyshap)
https://hg.mozilla.org/mozilla-central/rev/bedb4f9707ce
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
No more crashes.
Flags: needinfo?(garyshap)
Depends on: 998734
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.