Closed Bug 1314125 Opened 9 years ago Closed 9 years ago

[e10s] PopupNotifications is race-y and can show up for background browser

Categories

(Toolkit Graveyard :: Notifications and Alerts, defect)

Product:
Toolkit Graveyard
Component:
Notifications and Alerts
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(firefox49 affected, firefox50 affected, firefox51+ fixed, firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox49 --- affected
firefox50 --- affected
firefox51 + fixed
firefox52 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

Details

Attachments

(2 files)

Backed out changeset 30e050c04c4e
58 bytes, text/x-review-board-request
Details
Bug 1314125 - Make PopupNotifications use tabbrowser's selectedBrowser frameLoader instead of docShellIsActive to determine the currently selected tab.
58 bytes, text/x-review-board-request
mikedeboer
: review+
gchang
: approval-mozilla-aurora-
gchang
: approval-mozilla-beta+
Details
STR: 1) In an e10s window, have a tab loaded at https://stallman.org/ 2) Note that stallman.org definitely doesn't load any Flash plugins, or plugins of any kind. 3) In a second tab, load http://sf.curbed.com/2016/10/5/13174942/piedmont-home-cable-car-house-sale, and then immediately switch back to the stallman.org tab 4) Repeat step 3 until bug appears Expected results: The PopupNotification for the Flash usage shouldn't show up on the stallman.org tab. Actual results: Periodically, the plugin PopupNotification will appear in the stallman.org tab. Sometimes this shows the doorhanger, and sometimes just the "Lego block" plugin icon. I think this is because the notification is triggered by something that only loads some of the times on that particular curbed.com page.
This appears to be a race due to the fact that PopupNotifications.jsm uses docShellIsActive to determine whether or not a tab is currently selected. This is race-y, because the async tab switcher used for e10s in tabbrowser.xml only sets docShellIsActive to false once the layers have unloaded (or we've timed out waiting for layers to unload). This means that there's a pocket of time where a background tab is considered "selected" by PopupNotifications.jsm.
Assignee: nobody → mconley
Blocks: 1308677
Note that the first patch is a partial backout of bug 1254104
(I'm pretty sure that's okay, since it was to add PopupNotifications support for the special Hello window, which has been removed).
Attachment #8806128 - Flags: review?(gijskruitbosch+bugs) → review?(mdeboer)
Comment on attachment 8806128 [details] Bug 1314125 - Make PopupNotifications use tabbrowser's selectedBrowser frameLoader instead of docShellIsActive to determine the currently selected tab. https://reviewboard.mozilla.org/r/89628/#review89062 Yeah, this was for Hello specifically at the time. Having PopupNotifications.jsm married to tabbrowser this intimately might be practical, but is semantically incorrect. Just sayin'.
Attachment #8806128 - Flags: review?(mdeboer) → review+
(In reply to Mike de Boer [:mikedeboer] from comment #6) > > Yeah, this was for Hello specifically at the time. Having > PopupNotifications.jsm married to tabbrowser this intimately might be > practical, but is semantically incorrect. Just sayin'. I hear ya - but it's even part of the constructor: http://searchfox.org/mozilla-central/rev/021e8e0cee4f446757b8b1ffd312272174d6fc7b/toolkit/modules/PopupNotifications.jsm#194 So I think if somebody wants to use this thing, they have to supply some kind of tabbrowser, and that's kinda always been the case. If we want to make this more generic, there definitely deeper problems in this code. :) Thanks for the quick review!
Comment on attachment 8806127 [details] Backed out changeset 30e050c04c4e https://reviewboard.mozilla.org/r/89626/#review89280 This is a backout.
Attachment #8806127 - Flags: review+
Pushed by mconley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bd45588af02e Make PopupNotifications use tabbrowser's selectedBrowser instead of docShellIsActive to determine the currently selected tab. r=mikedeboer
Backed out for timing out in browser_permission_doorhanger.js on e10s: https://hg.mozilla.org/integration/autoland/rev/53b4f35c5830850f9609a451a139bffcf93233c2 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=bd45588af02e58ab94344d352e8bd09d4125d06b Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=5965884&repo=autoland 09:09:16 INFO - TEST-START | devtools/client/responsive.html/test/browser/browser_permission_doorhanger.js 09:09:16 INFO - Entering test bound 09:09:16 INFO - Adding a new tab with URL: http://example.com/ 09:09:17 INFO - Tab added and finished loading 09:09:17 INFO - TEST-PASS | devtools/client/responsive.html/test/browser/browser_permission_doorhanger.js | Permission doorhanger appeared without RDM enabled - 09:09:18 INFO - Opening responsive design mode 09:09:21 INFO - JavaScript error: resource://app/modules/ReaderParent.jsm, line 85: TypeError: win.gBrowser is undefined 09:09:21 INFO - JavaScript error: resource://gre/modules/RemoteAddonsParent.jsm, line 1078: TypeError: cannot use the given object as a weak map key 09:09:21 INFO - Console message: [JavaScript Error: "TypeError: win.gBrowser is undefined" {file: "resource://app/modules/ReaderParent.jsm" line: 85}] 09:09:21 INFO - Console message: [JavaScript Error: "TypeError: cannot use the given object as a weak map key" {file: "resource://gre/modules/RemoteAddonsParent.jsm" line: 1078}] 09:09:21 INFO - Responsive design mode opened 09:09:21 INFO - JavaScript error: resource://app/modules/ReaderParent.jsm, line 85: TypeError: win.gBrowser is undefined 09:09:21 INFO - Console message: [JavaScript Error: "TypeError: win.gBrowser is undefined" {file: "resource://app/modules/ReaderParent.jsm" line: 85}] 09:09:21 INFO - WARNING: content window passed to PrivateBrowsingUtils.isWindowPrivate. Use isContentWindowPrivate instead (but only for frame scripts). 09:09:21 INFO - pbu_isWindowPrivate@resource://gre/modules/PrivateBrowsingUtils.jsm:25:14 09:09:21 INFO - prompt@resource://app/modules/PermissionUI.jsm:291:11 09:09:21 INFO - prompt@resource://app/components/nsBrowserGlue.js:2555:7 09:09:21 INFO - WARNING: content window passed to PrivateBrowsingUtils.isWindowPrivate. Use isContentWindowPrivate instead (but only for frame scripts). 09:09:21 INFO - pbu_isWindowPrivate@resource://gre/modules/PrivateBrowsingUtils.jsm:25:14 09:09:21 INFO - prompt@resource://app/modules/PermissionUI.jsm:291:11 09:09:21 INFO - prompt@resource://app/components/nsBrowserGlue.js:2555:7 09:09:21 INFO - WARNING: content window passed to PrivateBrowsingUtils.isWindowPrivate. Use isContentWindowPrivate instead (but only for frame scripts). 09:09:21 INFO - pbu_isWindowPrivate@resource://gre/modules/PrivateBrowsingUtils.jsm:25:14 09:09:21 INFO - prompt@resource://app/modules/PermissionUI.jsm:291:11 09:09:21 INFO - prompt@resource://app/components/nsBrowserGlue.js:2555:7 09:09:21 INFO - WARNING: content window passed to PrivateBrowsingUtils.isWindowPrivate. Use isContentWindowPrivate instead (but only for frame scripts). 09:09:21 INFO - pbu_isWindowPrivate@resource://gre/modules/PrivateBrowsingUtils.jsm:25:14 09:09:21 INFO - Notification@resource://gre/modules/PopupNotifications.jsm:91:20 09:09:21 INFO - PopupNotifications_show@resource://gre/modules/PopupNotifications.jsm:409:24 09:09:21 INFO - prompt@resource://app/modules/PermissionUI.jsm:339:5 09:09:21 INFO - prompt@resource://app/components/nsBrowserGlue.js:2555:7 09:10:01 INFO - TEST-INFO | started process screentopng 09:10:03 INFO - TEST-INFO | screentopng: exit 0 09:10:03 INFO - checking window state 09:10:03 INFO - Console message: [JavaScript Error: "TypeError: this.globals.get(...) is undefined" {file: "resource://gre/modules/ExtensionContent.jsm" line: 951}] 09:10:03 INFO - TEST-UNEXPECTED-FAIL | devtools/client/responsive.html/test/browser/browser_permission_doorhanger.js | Test timed out - 09:10:03 INFO - Removing tab. 09:10:03 INFO - Waiting for event: 'TabClose' on [object XULElement]. 09:10:03 INFO - Got event: 'TabClose' on [object XULElement]. 09:10:03 INFO - Tab removed and finished closing
Flags: needinfo?(mconley)
And, though our wheels grind exceeding slow, eventually backed out the backout as well so we would actually stop timing out, https://hg.mozilla.org/integration/autoland/rev/561796983216c6941ac747d83c2bf79f4d5fd68b
Comment on attachment 8806128 [details] Bug 1314125 - Make PopupNotifications use tabbrowser's selectedBrowser frameLoader instead of docShellIsActive to determine the currently selected tab. This fixes the test failure that backed me out. Surprise: RDM uses some witchcraft to make the inner <iframe mozbrowser> that it's using to host the RDM content seem like the actual tab <xul:browser> (see http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/devtools/client/responsive.html/browser/tunnel.js#41) Re-requesting review to see if you have a better idea than comparing against frameLoader. I considered using permanentKey, but that seemed too SessionStore-specific. Not sure if it matters - both work.
Flags: needinfo?(mconley)
Attachment #8806128 - Flags: review+ → review?(mdeboer)
Comment on attachment 8806128 [details] Bug 1314125 - Make PopupNotifications use tabbrowser's selectedBrowser frameLoader instead of docShellIsActive to determine the currently selected tab. https://reviewboard.mozilla.org/r/89628/#review89792 Woah, that must've been a fun ride to figure out! ::: toolkit/modules/PopupNotifications.jsm:1057 (Diff revision 2) > anchors.add(defaultAnchor); > return anchors; > }, > > _isActiveBrowser: function (browser) { > - // Note: This helper only exists, because in e10s builds, > + // We compare on permanentKey instead of just comparing the s/permanentKey/frameLoader/ ::: toolkit/modules/PopupNotifications.jsm:1066 (Diff revision 2) > - : (this.window.gBrowser.selectedBrowser == browser); > + // calls from the tab to that iframe. This is so that attempts > + // to reload the tab end up reloading the content in > + // Responsive Design Mode, and not the Responsive Design Mode > + // viewer itself. > + // > + // This means that PopupNotification's can come up from a browser nit: PopupNotifications
Attachment #8806128 - Flags: review?(mdeboer) → review+
Comment on attachment 8806128 [details] Bug 1314125 - Make PopupNotifications use tabbrowser's selectedBrowser frameLoader instead of docShellIsActive to determine the currently selected tab. https://reviewboard.mozilla.org/r/89628/#review89792 Thanks for the super-quick review!
Pushed by mconley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2d12319aff83 Make PopupNotifications use tabbrowser's selectedBrowser frameLoader instead of docShellIsActive to determine the currently selected tab. r=mikedeboer
[Tracking Requested - why for this release]: There's a small window of time where background tabs might cause a PopupNotification (these are the doorhangers that appear for things like "Click to Play", for example) to appear for the fore-ground tab with e10s enabled.
status-firefox49: --- → affected
status-firefox50: --- → affected
status-firefox51: --- → affected
tracking-firefox51: --- → ?
I suspect I'm too late to try to get this into 50, but I think this patch is safe enough to land in 51.
Track 51+ as the incorrect behavior of push notification in tabs.
tracking-firefox51: ? → +
https://hg.mozilla.org/mozilla-central/rev/2d12319aff83
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
ni'ing myself for uplift requests once we're sure this hasn't introduced any new regressions.
Flags: needinfo?(mconley)
(In reply to Mike Conley (:mconley) - (digging out of review / needinfo backlog) from comment #24) > ni'ing myself for uplift requests once we're sure this hasn't introduced any > new regressions. Just so you're aware for uplifting: RDM only added the exposed `frameLoader` in 52, so you might see issues with the RDM test again. If this happens, I would recommend just disabling the affected RDM tests as part of your uplift because the affected RDM code will only be enabled for 52 and later.
Blocks: 1315317
Comment on attachment 8806128 [details] Bug 1314125 - Make PopupNotifications use tabbrowser's selectedBrowser frameLoader instead of docShellIsActive to determine the currently selected tab. Approval Request Comment [Feature/regressing bug #]: Bug 1254104 [User impact if declined]: There's a small window of time where, with e10s enabled, a PopupNotification doorhanger for a background tab will appear for the foreground tab after a tab switch. [Describe test coverage new/current, TreeHerder]: There are a number of tests for PopupNotifications, which all pass. This has also been in Nightly for a number of days. [Risks and why]: I'd say this is low risk. [String/UUID change made/needed]: None.
Flags: needinfo?(mconley)
Attachment #8806128 - Flags: approval-mozilla-beta?
Attachment #8806128 - Flags: approval-mozilla-aurora?
Comment on attachment 8806128 [details] Bug 1314125 - Make PopupNotifications use tabbrowser's selectedBrowser frameLoader instead of docShellIsActive to determine the currently selected tab. Since merge day is passed, 52 is aurora now. No need to uplift to 52 aurora. Remove the aurora flag. Fix an issue that the notification may be shown in the wrong tab. Beta51+. Should be in 51 beta 2.
Attachment #8806128 - Flags: approval-mozilla-beta?
Attachment #8806128 - Flags: approval-mozilla-beta+
Attachment #8806128 - Flags: approval-mozilla-aurora?
Attachment #8806128 - Flags: approval-mozilla-aurora-
has problems to apply: merging toolkit/modules/PopupNotifications.jsm warning: conflicts while merging toolkit/modules/PopupNotifications.jsm! (edit, then use 'hg resolve --mark') abort: unresolved conflicts, can't continue (use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(mconley)
to beta thats it :)
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #31) > Backed out from beta bug 1315317, bug 1314125 and the backout of bug 1254104 > for timeout in > devtools/client/responsive.html/test/browser/browser_permission_doorhanger. > js: See my comment 25, I recommend disabling this RDM test in 51 as part of your patch here.
Flags: needinfo?(mconley)
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: