Closed Bug 1294576 Opened 3 years ago Closed 3 years ago

[e10s] Red pulsing camera icon for subframe remains in address bar after main page refresh in Nightly

Categories

(Firefox :: Site Permissions, defect, P2)

All
macOS
defect

Tracking

()

VERIFIED FIXED
Firefox 51
Iteration:
51.3 - Sep 19
Tracking Status
e10s + ---
firefox51 --- verified

People

(Reporter: jib, Assigned: florian)

References

Details

(Keywords: regression, Whiteboard: [fxprivacy])

Attachments

(1 file, 1 obsolete file)

STR:
1. Open http://jsfiddle.net/srn9db4h/
2. Observe grey camera icon in address bar and permission request.
3. Click "Share Selected Devices".
4. Observe pulsing red camera icon in address bar.
5. Refresh the page.

Expected result:
- Red pulsing camera icon disappears. Goto 2.

Actual result:
- Red pulsing camera icon remains. Goto 2 (i.e. both red and gray icons).

This misbehavior is a regression that appears to have arrived with the new pulsing look.
Summary: Red flashing camera icon stuck in address bar after refresh → Red flashing camera icon stuck in address bar after refresh in Nightly
Summary: Red flashing camera icon stuck in address bar after refresh in Nightly → Red pulsing camera icon stuck in address bar after refresh in Nightly
Workaround: Selecting "Don't Share" in the new prompt makes both icons go away.
Summary: Red pulsing camera icon stuck in address bar after refresh in Nightly → Red pulsing camera icon stuck in address bar after page refresh in Nightly
Summary: Red pulsing camera icon stuck in address bar after page refresh in Nightly → Red pulsing camera icon remains in address bar after page refresh in Nightly
Unfortunately my test profile had e10s disabled. I can reproduce only if I enable e10s. I'm really surprised by this though, as I thought this was covered by automated tests.
Summary: Red pulsing camera icon remains in address bar after page refresh in Nightly → [e10s] Red pulsing camera icon remains in address bar after page refresh in Nightly
Whiteboard: [fxprivacy][triage]
So more details here:
- This only happens when the device is used by a subframe.
- Reloading the frame itself correctly clears the sharing indicator, it's only reloading the whole page that causes the indicator to remain visible.
Priority: -- → P2
Whiteboard: [fxprivacy][triage] → [fxprivacy]
In the specific case where the bug is reproduced, this test doesn't pass and the message is never sent to the UI: http://searchfox.org/mozilla-central/rev/b92ae9a263a05efefc0110eaad5e56369c9f1b10/browser/modules/ContentWebRTC.jsm#323

So it's likely that the changes in bug 1206233 made visible a bug in an edge case where it wasn't noticeable before (reloading the whole page clears all PopupNotification icons).
Blocks: 1206233
Summary: [e10s] Red pulsing camera icon remains in address bar after page refresh in Nightly → [e10s] Red pulsing camera icon for subframe remains in address bar after main page refresh in Nightly
Iteration: --- → 51.3 - Sep 12
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
Attached patch Patch (obsolete) — Splinter Review
When the sharing indicator was a PopupNotification, the indicator got dropped automatically by the PopupNotifications.locationChange call... and the reference to the stream and browser remained in the webrtcUI module. This patch cleans up more explicitly.
Attachment #8788929 - Flags: review?(jhofmann)
Assignee: nobody → florian
Status: NEW → ASSIGNED
Comment on attachment 8788929 [details] [diff] [review]
Patch

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

::: browser/base/content/test/webrtc/browser_devices_get_user_media_in_frame.js
@@ +213,5 @@
> +      PopupNotifications.panel.firstChild.button.click();
> +    });
> +    yield expectObserverCalled("getUserMedia:response:allow");
> +    yield expectObserverCalled("recording-device-events");
> +    is((yield getMediaCaptureState()), "CameraAndMicrophone",

nit: does (yield getMediaCaptureState()) really have to be in parentheses here?

::: browser/modules/webrtcUI.jsm
@@ +128,5 @@
>      }
>    },
>  
> +  forgetStreamsFromBrowser: function(aBrowser) {
> +    let found = false;

I think you can remove this :)

@@ +134,5 @@
> +    this._streams = this._streams.filter(stream => stream.browser != aBrowser);
> +    if (this._streams.length < length) {
> +      let tabbrowser = aBrowser.ownerGlobal.gBrowser;
> +      if (tabbrowser)
> +        tabbrowser.setBrowserSharing(aBrowser, {});

I don't understand why we need to make sure we actually removed some streams to reset the sharingState of the tab. In browser.js we already check if the tab has a sharingState. Shouldn't we reset this in any case?

This patch would be much smaller (and IMO clearer) if you could skip the checks and move this line to browser.js.
Attachment #8788929 - Flags: review?(jhofmann)
Attached patch Patch v2Splinter Review
(In reply to Johann Hofmann [:johannh] from comment #7)

> browser/base/content/test/webrtc/browser_devices_get_user_media_in_frame.js
> @@ +213,5 @@
> > +      PopupNotifications.panel.firstChild.button.click();
> > +    });
> > +    yield expectObserverCalled("getUserMedia:response:allow");
> > +    yield expectObserverCalled("recording-device-events");
> > +    is((yield getMediaCaptureState()), "CameraAndMicrophone",
>
> nit: does (yield getMediaCaptureState()) really have to be in parentheses
> here?

Probably not. I vaguely remember adding these parentheses to make a linter happy, but I can't reproduce it now... Still I would prefer to not change it here as I prefer to keep the file consistent (it was just a copy/paste here).
Attachment #8789313 - Flags: review?(jhofmann)
Attachment #8788929 - Attachment is obsolete: true
Comment on attachment 8789313 [details] [diff] [review]
Patch v2

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

Looks good, thanks!
Attachment #8789313 - Flags: review?(jhofmann) → review+
https://hg.mozilla.org/integration/fx-team/rev/beda515683970341d1338dbc687bb929c0ce291f
Bug 1294576 - Remove device sharing indicator on location changes, r=johannh.
https://hg.mozilla.org/mozilla-central/rev/beda51568397
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
QA Contact: paul.silaghi
Verified fixed FX 51.0a1 (2016-09-14) Win 7
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.