webrtcUI.jsm (handleRequest): can't access dead object

RESOLVED FIXED in Firefox 28

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jruderman, Assigned: jib)

Tracking

(Blocks 1 bug, {testcase})

Trunk
mozilla30
x86_64
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox28 fixed, firefox29 fixed, firefox30 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Posted file testcase
JavaScript error: resource://app/modules/webrtcUI.jsm, line 73: can't access dead object
Note that when I run the testcase, I get a different result: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIURI.host] , so it looks like there's a race here.

But going by your findings:

Looks like your testcase changes window.location to something
 nonsensical, and this asynchronous callback in webrtcUI.jsm uses the aSubject.windowId that was originally passed in in line 67, in line 73 which is a callback that happens later after the window is gone because of your location change:

67> function handleRequest(aSubject, aTopic, aData) {
68    let constraints = aSubject.getConstraints();
69 
70    Services.wm.getMostRecentWindow(null).navigator.mozGetUserMediaDevices(
71      constraints,
72      function (devices) {
73>       prompt(aSubject.windowID, aSubject.callID, constraints.audio,
74               constraints.video || constraints.picture, devices);

While we could patch the code to check for this http://stackoverflow.com/questions/18401890/know-if-an-object-is-dead I suppose the question is if there's a larger problem here of all our asynchronous callbacks being vulnerable to DOM changes in a way that could cause misbehavior or mischief.

Perhaps there's a more general fix possible to somehow make our asynchronous callbacks fall to the floor when the window is torn down. E.g. make a callback-class that checks synchronously for teardown right before calling the callback.
I suppose this problem is limited to browser-chome callbacks, since all content callbacks are gone with the old window, so perhaps checking just this instance is sufficient.
This turned out to be unrelated to Bug 919244 in the end.
Cleanup that avoids the dead object situation.
Attachment #8375840 - Flags: review?(rjesup)
Attachment #8375840 - Flags: review?(rjesup) → review+
Updated commit message. Carrying forward r+ from jesup.
Attachment #8375840 - Attachment is obsolete: true
Attachment #8375895 - Flags: review+
Keywords: checkin-needed
Fixes android bustage. Carrying forward r+ from jesup.

Try - https://tbpl.mozilla.org/?tree=Try&rev=c9faa100481d
Attachment #8375895 - Attachment is obsolete: true
Attachment #8376483 - Flags: review+
Keywords: checkin-needed
Blocks: 973318
https://hg.mozilla.org/mozilla-central/rev/3576a62195ec
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Comment on attachment 8376483 [details] [diff] [review]
GetUserMediaDevices cleanup + untie GetUserMediaRequest obj from DOM (2). r=jesup

[Approval Request Comment]
Bug caused by (feature/regressing bug #): gUM permission prompt
User impact if declined: Dead object.

Testing completed (on m-c, etc.): 
Fix verified by schien (b2g) and myself (desktop) independently. Landed on m-c a week ago.

Risk to taking this patch (and alternatives if risky): Low

String or IDL/UUID changes made by this patch: none
Attachment #8376483 - Flags: approval-mozilla-aurora?
Attachment #8376483 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: checkin-needed
Comment on attachment 8376483 [details] [diff] [review]
GetUserMediaDevices cleanup + untie GetUserMediaRequest obj from DOM (2). r=jesup

[Approval Request Comment]
Bug caused by (feature/regressing bug #): gUM permission prompt
User impact if declined: Dead object.

Testing completed (on m-c, etc.): 
Fix verified by schien (b2g) and myself (desktop) independently. Landed on m-c a week ago, aurora this morning.

Risk to taking this patch (and alternatives if risky): Low
Attachment #8376483 - Flags: approval-mozilla-beta?
Attachment #8376483 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: in-testsuite?
Keywords: testcase
You need to log in before you can comment on or make changes to this bug.