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.

I would assume some other window level ui elements may also need updating when swapping docshells. e.g. the camera menubutton

(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.

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.

(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.

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.

(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?

(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.

(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?

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.

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.

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.

(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?

(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).

OK, can we just move it to bug 723951 then?

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.