Closed Bug 1325223 Opened 3 years ago Closed 3 years ago

PopupNotifications triggers showing/shown events multiple times

Categories

(Toolkit :: XUL Widgets, defect, P2)

53 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 --- fixed

People

(Reporter: aswan, Assigned: florian)

References

Details

(Keywords: regression, Whiteboard: [fxprivacy])

Attachments

(2 files)

If a doorhanger is created using PopupNotifications with the `{permanent: true}` option, the doorhanger remains visible even if focus changes away from the browser and back.  But in this case, the popup's `eventCallback` function is called with the "showing" and "shown" events on the focus change.  As a result, it is easy for code that runs during either of those events that adds DOM elements to accidentally run multiple times and add extra unwanted elements.
These events appear to be intended to only run once each, this bug is to restore that behavior.
Component: Site Identity and Permission Panels → XUL Widgets
Product: Firefox → Toolkit
Whiteboard: [fxprivacy] [triage]
(In reply to Andrew Swan [:aswan] from comment #0)

> These events appear to be intended to only run once each, this bug is to
> restore that behavior.

It's fine for these events to run several times, but there needs to be a dismissed event in between to give the code an opportunity to cleanup whatever has been created while showing the notification before.

It's expected to have a dismissed event followed by showing & shown when the user switches back and forth between tabs.

The problem here is that when the browser loses focus, we don't hide persistent notifications, but we still try to re-show them when the browser is re-focused.
Blocks: 1004061
This bug is being worked around in bug 1308309, and I would have worked around it too in bug 1284877 if I had noticed at the time. Currently I think this keeps a video stream around (hopefully only until the next GC, but I'm not sure) whenever the browser is unfocused and refocused while a screensharing preview was displayed.
Priority: -- → P2
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
(In reply to Florian Quèze [:florian] [:flo] from comment #2)
> This bug is being worked around in bug 1308309, and I would have worked
> around it too in bug 1284877 if I had noticed at the time. Currently I think
> this keeps a video stream around (hopefully only until the next GC, but I'm
> not sure) whenever the browser is unfocused and refocused while a
> screensharing preview was displayed.

Ouch.  Can we work around it now?
Flags: needinfo?(florian)
(In reply to Randell Jesup [:jesup] from comment #3)
> (In reply to Florian Quèze [:florian] [:flo] from comment #2)
> > This bug is being worked around in bug 1308309, and I would have worked
> > around it too in bug 1284877 if I had noticed at the time. Currently I think
> > this keeps a video stream around (hopefully only until the next GC, but I'm
> > not sure) whenever the browser is unfocused and refocused while a
> > screensharing preview was displayed.
> 
> Ouch.  Can we work around it now?

I would rather fix it for real. But it turns out currently we don't leak a stream, because we don't get a stream at all, due to bug 1320170.
Flags: needinfo?(florian)
Attached patch PatchSplinter Review
Attachment #8828923 - Flags: review?(past)
Assignee: nobody → florian
Status: NEW → ASSIGNED
Comment on attachment 8828923 [details] [diff] [review]
Patch

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

Thanks!
Attachment #8828923 - Flags: review?(past) → review+
With some manual testing, confirmed that the workarounds in ExtensionsUI.jsm
that originally prompted filing this bug are no longer needed.  This patch
removes those workarounds, feel free to roll it into this bug, or I can
push it separately if that's easier.  But regardless, hurry for reducing
entropy!
Assignee: florian → aswan
bzexport got a little carried away, I didn't mean to take this bug
Assignee: aswan → florian
Attachment #8828942 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/98c3bfd85ae321add710899c3203498b9ec60ec6
Bug 1325223 - Focusing the window shouldn't trigger the showing/shown events again on persistent notifications that were never dismissed, r=past.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bfa82216639a496a20b05319d0c001ce1892e5f
Bug 1325223 - Remove ExtensionsUI.jsm workarounds, r=florian.
https://hg.mozilla.org/mozilla-central/rev/98c3bfd85ae3
https://hg.mozilla.org/mozilla-central/rev/4bfa82216639
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Are any older releases affected or was 53 the first?
Flags: needinfo?(florian)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)
> Are any older releases affected or was 53 the first?

The issue was introduced by bug 1004061, so 53 was the first.
Flags: needinfo?(florian)
Version: unspecified → 53 Branch
Depends on: 1336066
Duplicate of this bug: 1336067
Setting qe-verify- since this fix seems to have automated coverage. Florian, if you think manual QA should instead be looking at this, feel free to flip the flag or ni? me.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.