Closed Bug 1203900 Opened 4 years ago Closed 4 years ago

Fix cycle collection and array buffer creation bug in the implementation of MediaKeyMessageEvent, MediaEncryptedEvent, and BluetoothLeDeviceEvent

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: yrliou, Assigned: yrliou)

References

Details

Attachments

(1 file, 1 obsolete file)

During the review of Bug 1181482, we found bugs in the cycle collection part and array buffer creation in the implementation of MediaKeyMessageEvent, MediaEncryptedEvent, and BluetoothLeDeviceEvent.
Details has been described in comment 19-21 of Bug 1181482.
Hi Boris,

This patch is fixing the bug we found when you reviewed Bug 1181482, could you help to review it?

Thanks,
Jocelyn
Attachment #8659826 - Flags: review?(bzbarsky)
Comment on attachment 8659826 [details] [diff] [review]
Bug 1203900 - Fix cycle collection and array buffer creation bug in the implementation of MediaKeyMessageEvent, MediaEncryptedEvent, and BluetoothLeDeviceEvent.

r=me, though I double-checked with mccr8 and the NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS is _not_ in fact needed here (though it also doesn't hurt), because the trace implementation for Event calls it already.  That means there is no leak right now, so hard to write tests for one.  ;)
Attachment #8659826 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #2)
> Comment on attachment 8659826 [details] [diff] [review]
> Bug 1203900 - Fix cycle collection and array buffer creation bug in the
> implementation of MediaKeyMessageEvent, MediaEncryptedEvent, and
> BluetoothLeDeviceEvent.
> 
> r=me, though I double-checked with mccr8 and the
> NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS is _not_ in fact needed
> here (though it also doesn't hurt), because the trace implementation for
> Event calls it already.  That means there is no leak right now, so hard to
> write tests for one.  ;)

Ah, thanks, sorry for not finding that out.
If so, I would like to remove it.
And also remove it in Bug 1181482, is the leak test still needed?
> If so, I would like to remove it.

Makes sense.

> And also remove it in Bug 1181482

Yep.

> is the leak test still needed?

No, since it wouldn't leak... ;)
Another clean try:
https://treeherder.mozilla.org/#/jobs?repo=try&tochange=48006a78be5a

Those fail items seem already there and unrelated to my patch.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9a3bfacb1a09
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.