1365852 - Intermittent browser/base/content/test/webrtc/browser_devices_get_user_media_unprompted_access_queue_request.js | leaked 1 window(s) until shutdown [url = chrome://browser/content/webrtcIndicator.xul]

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P3)

Comment 1

[Byron Campen [:bwc]]

It is unclear how common this intermittent failure is...

Comment 2

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bbe25c2e4aef2b8f97d2748b592502c4781fe12&selectedJob=103304939

Comment 3

[Florian Quèze [:florian]]

(In reply to Munro Mengjue Chiang [:mchiang] from comment #2)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=4bbe25c2e4aef2b8f97d2748b592502c4781fe12&selectedJob=1
> 03304939

Looks like it's failing all the time. This leak can't exist on Mac because the webrtcIndicator.xul window doesn't exist there, the global sharing indicators are icons in the menubar instead.

Comment 4

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Still have no clue. This bug only occurs on try machine.

Comment 5

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Please refer to this log
https://treeherder.mozilla.org/logviewer.html#?job_id=104826384&repo=try&lineNumber=2933

When this leakage issue occurs, one thread is blocking on this function gIndicatorWindow.updateIndicatorState()
https://hg.mozilla.org/try/file/b29733b2def9/browser/modules/webrtcUI.jsm#l1090

Florian, do you have idea why the function gIndicatorWindow.updateIndicatorState() never returns?

Comment 6

[Florian Quèze [:florian]]

(In reply to Munro Mengjue Chiang [:mchiang] from comment #5)
> Please refer to this log
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=104826384&repo=try&lineNumber=2933
> 
> When this leakage issue occurs, one thread is blocking on this function
> gIndicatorWindow.updateIndicatorState()
> https://hg.mozilla.org/try/file/b29733b2def9/browser/modules/webrtcUI.
> jsm#l1090
> 
> Florian, do you have idea why the function
> gIndicatorWindow.updateIndicatorState() never returns?

It likely throws an exception. You can put a try {...} catch(e) {ok(false, e);} around it to see what the exception is.

Comment 7

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

After adding try catch, this phenomenon suddenly disappears.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd5f0740aea50c582158dfe27ccfa9dee312997c&selectedJob=104956333
I check each job in the request and none of them throws an exception.
This might be a timing issue?

Comment 8

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

As mentioned in bug 861716 comment 101, there is a bug that we may call updateIndicatorState before webrtcIndicator is loaded & initialized and cause an exception.

Queuing gUM request in MediaManager (introduced in bug 861716) and unprompted access (introduced in bug 1270572) raise the reproduce rate of this bug.

Comment 9

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug1365852-ensure-webrtcIndicator-init-done-before-calling-updateIndicatorState.patch

Comment 10

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=045c7f21056605060a1ae4c456abaabd6c6bbadd

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8876971 [details] [diff] [review]
Bug1365852-ensure-webrtcIndicator-init-done-before-calling-updateIndicatorState.patch

Review of attachment 8876971 [details] [diff] [review]:
-----------------------------------------------------------------

Please provide patches with 8 lines of context.

I think the easiest fix here would be to just add:

// If stringBundle isn't set, the window hasn't finished loading.
if (!stringBundle)
  return;

at http://searchfox.org/mozilla-central/rev/61054508641ee76f9c49bcf7303ef3cfb6b410d2/browser/base/content/webrtcIndicator.js#42

::: browser/modules/webrtcUI.jsm
@@ +1085,5 @@
> +      if (AppConstants.platform == "macosx" || gIndicatorWindowInited) {
> +        updateIndicatorState();
> +      } else {
> +        gIndicatorWindow.addEventListener("load", () => {
> +          updateIndicatorState();

You don't need this, see
http://searchfox.org/mozilla-central/rev/61054508641ee76f9c49bcf7303ef3cfb6b410d2/browser/base/content/webrtcIndicator.xul#17
and
http://searchfox.org/mozilla-central/rev/61054508641ee76f9c49bcf7303ef3cfb6b410d2/browser/base/content/webrtcIndicator.js#32

Comment 12

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug 1365852 - immediately return in updateIndicatorState() if webrtcIndicator hasn't finished loading.

Review commit: https://reviewboard.mozilla.org/r/148764/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148764/

Comment 13

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=525042d984ca6c2fee4b9ddc25ce0480bb3ee610

Comment 14

[Florian Quèze [:florian]]

Comment on attachment 8877384 [details]
Bug 1365852 - immediately return in updateIndicatorState() if webrtcIndicator hasn't finished loading.

https://reviewboard.mozilla.org/r/148764/#review153378

Comment 15

[Mozilla Autoland]

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 1 changes to 1 files
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Rev 85bf4deb567e needs "Bug N" or "No bug" in the commit message.
remote: Munro Mengjue Chiang <[e-mail]>
remote: Bug1365852 - immediately return in updateIndicatorState() if webrtcIndicator hasn't finished loading. r=florian
remote: 
remote: MozReview-Commit-ID: D6MZn6khaJO
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.c_commitmessage hook failed
abort: push failed on remote

Comment 16

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8877384 [details]
Bug 1365852 - immediately return in updateIndicatorState() if webrtcIndicator hasn't finished loading.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148764/diff/1-2/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91908f42965c
immediately return in updateIndicatorState() if webrtcIndicator hasn't finished loading. r=florian

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/91908f42965c