Closed
Bug 883326
Opened 11 years ago
Closed 11 years ago
swapping docshells does not swap popup notifications
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
DUPLICATE
of bug 723951
People
(Reporter: mixedpuppy, Assigned: florian)
References
Details
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
If you have a tab that has dismissed notifications (ie. you didn't grant or deny the permission, just clicked away) then you drag that tab out to a new window, the notifications are lost.
I started working on this as a part of bug 880911 and bug 809085 (though this issue is generic) and got part way (see attached patch, notifications are swapped to new browser). What I realized is that some notifications modify their panel prior to calling PopupNotifications.show (e.g. webrtc, see prompt() in browser/modules/webrtcUI.jsm). To make this work, we'll need a way to callback to any prompts that do this kind of modification so the new window is setup properly.
Reporter | ||
Comment 1•11 years ago
|
||
I would assume some other window level ui elements may also need updating when swapping docshells. e.g. the camera menubutton
Assignee | ||
Comment 2•11 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #0)
> To make this work, we'll
> need a way to callback to any prompts that do this kind of modification so
> the new window is setup properly.
An alternative is to do these modifications during the "showing" event of the notification.
The attached work in progress works for the (WebRTC) device icon in regular browser tabs. Next steps are to make this work in SocialAPI chat windows, and check if other notifications need similar changes.
Feedback welcome.
Assignee | ||
Comment 3•11 years ago
|
||
This now works in the SocialAPI chat windows. Not ready for review yet, I still need to look into some memory leaks, check if there are other kinds of notifications that need code moved to the 'showing' event, and see if tests need to be adapted/written.
Assignee: nobody → florian
Attachment #762854 -
Attachment is obsolete: true
Attachment #797914 -
Attachment is obsolete: true
Assignee | ||
Comment 4•11 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #3)
> I still need to look into some memory leaks
The shutdown leak I was observing on my debug build seemed more related to pending WebRTC device permission request than to popup notifications.
> check if there are other kinds of
> notifications that need code moved to the 'showing' event
I found only one other notification (the mixed-content one) that touched the DOM, and it wasn't even really useful.
> and see if tests need to be adapted/written.
I added a test in browser_popupNotification.js.
Attachment #798552 -
Attachment is obsolete: true
Attachment #800833 -
Flags: review?(dao)
Comment 5•11 years ago
|
||
Comment on attachment 800833 [details] [diff] [review]
Patch v3
The first secondaryActions callback for mixed-content-blocked looks like this:
6608 callback: function() {
6609 // Use telemetry to measure how often unblocking happens
6610 const kMIXED_CONTENT_UNBLOCK_EVENT = 2;
6611 let histogram =
6612 Services.telemetry.getHistogramById("MIXED_CONTENT_UNBLOCK_COUNTER");
6613 histogram.add(kMIXED_CONTENT_UNBLOCK_EVENT);
6614 // Reload the page with the content unblocked
6615 BrowserReloadWithFlags(nsIWebNavigation.LOAD_FLAGS_ALLOW_MIXED_CONTENT);
6616 }
It doesn't look like this will work as intended when the notification was moved to another window, because this code would keep the original window as its context and call BrowserReloadWithFlags there.
Generally, it seems wrong that the code backing the notification can belong to a different chrome window than where the notification is showing. As I understand it, this would keep the original window alive even when it was closed.
Attachment #800833 -
Flags: review?(dao) → review-
Comment 6•11 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> The shutdown leak I was observing on my debug build seemed more related to
> pending WebRTC device permission request than to popup notifications.
Well, do you see the leak without your patch?
OS: Mac OS X → All
Hardware: x86 → All
Version: 23 Branch → Trunk
Updated•11 years ago
|
Blocks: doorhanger
Summary: swapping docshells does not swap notifications → swapping docshells does not swap popup notifications
Assignee | ||
Comment 7•11 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #6)
> (In reply to Florian Quèze [:florian] [:flo] from comment #4)
> > The shutdown leak I was observing on my debug build seemed more related to
> > pending WebRTC device permission request than to popup notifications.
>
> Well, do you see the leak without your patch?
Yes, it also happened without the patch. And unfortunately I don't have exact steps to reproduce.
While investigating leaks that could be caused by this patch, I also was under the impression at some point that the popup notification code was keeping the window alive, when it was actually another unrelated easily reproducible existing bug that I filed as bug 912047.
Assignee | ||
Comment 8•11 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #5)
> It doesn't look like this will work as intended when the notification was
> moved to another window, because this code would keep the original window as
> its context and call BrowserReloadWithFlags there.
Should this callback be created during the "showing" event like I did for the webrtc notifications?
> Generally, it seems wrong that the code backing the notification can belong
> to a different chrome window than where the notification is showing. As I
> understand it, this would keep the original window alive even when it was
> closed.
I was under the impression that as long as we don't have closures keeping references to objects that reference the initial window (like a browser DOM node would for example), we wouldn't keep the original window alive. BrowserReloadWithFlags seems like a problem in the code you quoted. Would window.BrowserReloadWithFlags be enough to avoid it? (Not sure if the global "window" object is handled similarly to the 'this' keyword, or like a regular variable.) Or would we have to find a way to access the current global object from a parameter of the callback?
Updated•11 years ago
|
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
Comment 10•11 years ago
|
||
We'll likely need some way for the PopupNotification consumer to be notified and handle docshell-swapping cases itself. I don't think it's possible to make it "just work" given arbitrary mainAction/secondAction handlers referencing the original objects directly or via a closure.
Assignee | ||
Comment 11•11 years ago
|
||
Comment on attachment 800833 [details] [diff] [review]
Patch v3
Note: I pushed this to try Friday (https://tbpl.mozilla.org/?tree=Try&rev=d86bb311e282) and it looks like it makes failures similar to bug 886995 happen frequently.
Assignee | ||
Comment 12•11 years ago
|
||
Thanks for the feedback. This is definitely related to bug 723951, but I don't think it's an exact duplicate. Bug 723951 seems to be mostly about tabs, and the work in progress there does things that are specific to tabs.
Tabs are only a subset of the problem I'm trying to address; the case I actually care strongly about doesn't involve tabs. The problem I have is that the device icons in SocialAPI floating chat windows are lost when detaching a floating chat window to a separate window. It's this specific use case that blocks Talkilla.
The updated patch I'm attaching sends an event to notifications to let them decide if they want to be swapped, and let them handle the swapping if needed. This makes the swapping of notifications opt-in for each notification, and the patch only adds support for swapping WebRTC device notifications (the case that blocks Talkilla). I think swapping of the other notifications can be handled in separate bugs.
Attachment #800833 -
Attachment is obsolete: true
Attachment #802253 -
Flags: review?(dao)
Attachment #802253 -
Flags: feedback?(gavin.sharp)
Comment 13•11 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #12)
> Thanks for the feedback. This is definitely related to bug 723951, but I
> don't think it's an exact duplicate. Bug 723951 seems to be mostly about
> tabs, and the work in progress there does things that are specific to tabs.
I don't see how we would fix one and not the other. Certainly the solution we come up for your case should be extendable to cover the tabs case too. Is it?
Assignee | ||
Comment 14•11 years ago
|
||
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #13)
> I don't see how we would fix one and not the other. Certainly the solution
> we come up for your case should be extendable to cover the tabs case too. Is
> it?
My patch fixes the tabs case too (but without doing anything that's tab-specific).
Comment 15•11 years ago
|
||
OK, can we just move it to bug 723951 then?
Assignee | ||
Comment 16•11 years ago
|
||
Comment on attachment 802253 [details] [diff] [review]
Patch v4
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #15)
> OK, can we just move it to bug 723951 then?
Done.
Attachment #802253 -
Flags: review?(dao)
Attachment #802253 -
Flags: feedback?(gavin.sharp)
You need to log in
before you can comment on or make changes to this bug.
Description
•