723951 - Popup notification does not continue when the tab move to other window.

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0a1) Gecko/20120202 Firefox/13.0a1
Build ID: 20120202031238

Steps to reproduce:

When the tab move to other window from the current window, Popup notification  doesn't continue.

Step to reproduce:
1. Open a new tab and show popup notification at the tab on window (A).
2. Move the tab to an other window (B) from the window (A).
This bug occurs in either case, whether the window which user moves the tab to is a new window or a window opened already.


Actual results:

Popup notification does not continue in the window (B).
This bug caused by not passing xul:browser.popupNotifications from window(A) to window(B).


Expected results:

Popup notifications continues in new window.

Comment 1

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: proposed patch v1.0

Comment 2

[Josh Matthews [:jdm]]

Comment on attachment 594191 [details] [diff] [review]
proposed patch v1.0

Thank you for the patch!

Comment 3

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: proposed patch v1.1

I fixed a few typos, the parameter name of function passed to otherBrowser.popupNotifications.forEach().
I did not change other part.

Comment 5

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: proposed patch v1.2

Rebase the patch on http://hg.mozilla.org/mozilla-central/rev/6fbaf8cf139c

Comment 6

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

When moving a tab from other window, call stack (from Venkman):

#0: function swapBrowsersAndCloseOther() in <chrome://browser/content/tabbrowser.xml> line 1871
#1: function BrowserStartup() in <chrome://browser/content/browser.js> line 4847
#2: function onload() in <chrome://browser/content/browser.xul> line 1

Comment 7

[Josh Matthews [:jdm]]

Comment on attachment 600674 [details] [diff] [review]
proposed patch v1.2

Let's see if Felipe can use his new review powers to speed this process along.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

This is going to be trickier than I thought - there are popupnotifications objects that hold on to their own windows via their "options" object, e.g. the XPI install ones:

http://hg.mozilla.org/mozilla-central/annotate/c4eb6151a3bf/browser/base/content/browser.js#l774

We can't reasonably update those references automatically, and not doing so will result in a broken notification, so the status quo (no notifications) seems like a better state...

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 600674 [details] [diff] [review]
proposed patch v1.2

This might need to be WONTFIX, unless we can figure out a way to have the notifications re-fire "naturally"... :(

Comment 10

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #8~#9)
> This is going to be trickier than I thought - there are popupnotifications
> objects that hold on to their own windows via their "options" object, e.g.
> the XPI install ones:
> 
> http://hg.mozilla.org/mozilla-central/annotate/c4eb6151a3bf/browser/base/
> content/browser.js#l774
> 
> We can't reasonably update those references automatically, and not doing so
> will result in a broken notification, so the status quo (no notifications)
> seems like a better state...
>
>
> This might need to be WONTFIX, unless we can figure out a way to have the
> notifications re-fire "naturally"... :(

Current Firefox can detach tab between another windows without reloading browser's content window. (This behavior is based on http://hg.mozilla.org/mozilla-central/rev/48191fdcf8a7, isn't it?)

Accordingly, I think that references of "options" object properties are updated automatically by existing implementation. In fact, with using my proposed patch v 1.2, it success the XPI install one when move to new window after starting XPI installation.

I think some approach to this bugs:

1. Use current approach.
This reason is above and this approach will be fix certainly. But this behavior may be tricky. If we apply this patch, other latent bugs may be found.

2. Use current approach but limit continued popup notifications.
This approach does not take off some risky notifications (e.g. XPI install).
So this approach take off *safe* notifications.
But this may be broken if user defined notifications are *risky*.

3. WONTFIX this bug & Make a way to re-fire the notifications "naturally":
This approach will cancel the notifications fired on old window.
For example:
1. Fire geolocation notification in tab which is on window(A).
2. Detach the tab from window(A), and Move to window(B).
3. Cancel geolocation notification as user canceled it.
This approach needs adding the canceling process to exiting notifications. And we may need to make a prompt for users.

Which approach should we take?

Comment 11

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

I thought about above approaches.
I note them:

> 1. Use current approach.
> This reason is above and this approach will be fix certainly. But this
> behavior may be tricky. If we apply this patch, other latent bugs may be
> found.

This approach will be tricky. Even if we fix this bug with this approach, the patch code will be tricky & this may leak objects. e.g. Gavin's comment( https://bugzilla.mozilla.org/show_bug.cgi?id=723951#c8)

> 2. Use current approach but limit continued popup notifications.
> This approach does not take off some risky notifications (e.g. XPI install).
> So this approach take off *safe* notifications.
> But this may be broken if user defined notifications are *risky*.

This approach will not be good. We cannot know all user defined *risky* notifications if we use a white list for this approach. As black list, we need to have the list. It's too bad.

> 3. WONTFIX this bug & Make a way to re-fire the notifications "naturally":
> This approach will cancel the notifications fired on old window.
> For example:
> 1. Fire geolocation notification in tab which is on window(A).
> 2. Detach the tab from window(A), and Move to window(B).
> 3. Cancel geolocation notification as user canceled it.
> This approach needs adding the canceling process to exiting notifications.
> And we may need to make a prompt for users.

I thought, implementing a new notification event which describes notifications are swapped from other window, and caring existing notifications with using the new event.

Comment 12

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

For implement a new notifications event (https://developer.mozilla.org/en/JavaScript_code_modules/PopupNotifications.jsm#Notification_events), we need an API to hook tabbrowser.swapBrowsersAndCloseOther() (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1836).
We can create the new notification event with changing only tabbrowser.swapBrowsersAndCloseOther(). But the notification event should be created in PopupNotifications.jsm.

Comment 13

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

I filed Bug 740362 for implement to hook tabbrowser.swapBrowsersAndCloseOther().

Comment 14

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: part1 - Enable to swap browser.popupNotifications

We need to swap browser.popupNotifications for treating them on new xul:browser element.

Comment 15

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: part2 - Implement notification event

Implement the notification event to notify notifications are swapped.
This patch is *working draft*.

The patch part 2 or later, it depend on bug 740362.
And we need to think the event name.

Comment 16

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: wip

I merges patches from bug 740369.

This approach need to handle the new event for existing doorhanger notifications.
But I have not implemented yet.

Comment 17

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #8)
> This is going to be trickier than I thought - there are popupnotifications
> objects that hold on to their own windows via their "options" object, e.g.
> the XPI install ones:
> 
> http://hg.mozilla.org/mozilla-central/annotate/c4eb6151a3bf/browser/base/
> content/browser.js#l774
> 
> We can't reasonably update those references automatically, and not doing so
> will result in a broken notification, so the status quo (no notifications)
> seems like a better state...

Gavin, I re-think about calling PopupNotifications.show() in mozilla sorce tree.
Your pointed tha part ( http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-addons.js#103 ) is an exception case, and its case will be resolved by intoroducing new popnotification event. (Its case releases the contentWindow when fire PopupNotification event callback).

Comment 18

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: wip v2

This wip does not work well in some cases which pass |xul:tabbrowser.updateCurrentBrowser()| because of swap docshell to new browser.

Do you have any good ideas?

Comment 19

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #18)
> Created attachment 690202 [details] [diff] [review]
> wip v2
> 
> This wip does not work well in some cases which pass
> |xul:tabbrowser.updateCurrentBrowser()| because of swap docshell to new
> browser.
> 
> Do you have any good ideas?

Sorry. This have my mis-understanding.
This is related to http://hg.mozilla.org/mozilla-central/file/3905010a2346/browser/base/content/browser.js#l4062.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 690202 [details] [diff] [review]
wip v2

This looks good - I had a few concerns:
- should TabImport be fired before _swapBrowserDocShells?
- can we fire TabImport on the old browser, rather than the new one, to remove the need to initialize PopupNotifications in the new window manually?

We'll also need to update http://hg.mozilla.org/mozilla-central/annotate/3905010a2346/browser/base/content/browser-addons.js#l93 to observe for the "swapped" event and update its "contentWindow" reference, right?

Comment 21

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Thank you feedback!

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #20)
>
> - should TabImport be fired before _swapBrowserDocShells?

(I have not tested yet, but) We can change it. However, it may not be useful when using TabImport event for other purpose. Firefox has not completed a swapping step before _swapBrowserDocShells.


> - can we fire TabImport on the old browser, rather than the new one, to
> remove the need to initialize PopupNotifications in the new window manually?

Umm... It might be difficult by the current design of PopupNotifications.
The current design need to listen "activate" event necessarily by an instance of PopupNotifications constructor in the new window, for showing doorhanger icon.
I'll try to think changing the design about updating & showing notifications.


> We'll also need to update
> http://hg.mozilla.org/mozilla-central/annotate/3905010a2346/browser/base/
> content/browser-addons.js#l93 to observe for the "swapped" event and update
> its "contentWindow" reference, right?

We need add the 3rd parameter as an object (Map, Array, or simple object) to PopupNotifications._fireCallback (http://hg.mozilla.org/mozilla-central/file/3905010a2346/toolkit/content/PopupNotifications.jsm#l593).
I think it will resolve this.

Comment 22

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: work in progress v2.1

Sorry. Gavin's feedback have not been reflected to this yet.

Change from wip v2:
* Fix that when notifications are not continued when swapping tab to a new window.

Comment 23

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #21)
> (In reply to :Gavin Sharp (use [e-mail] for email) from comment
> #20)
> > - can we fire TabImport on the old browser, rather than the new one, to
> > remove the need to initialize PopupNotifications in the new window manually?
> 
> Umm... It might be difficult by the current design of PopupNotifications.
> The current design need to listen "activate" event necessarily by an
> instance of PopupNotifications constructor in the new window, for showing
> doorhanger icon.
> I'll try to think changing the design about updating & showing notifications.

I thought about it. This is my thought:

Even if changing the design of PopupNotifications, we need to listen some kind of event/observer-notifying on a instance of PopupNotifications, in the new window.

Also, if we import the browser element which has some doorhanger from other window to the new one, the new one must handle its doorhanger notification in either case. So we need to accept this compelling call which initialize the instance of PopupNotification in the new window.

Comment 24

[YUKI "Piro" Hiroshi]

(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #21)
> (In reply to :Gavin Sharp (use [e-mail] for email) from comment
> #20)
> >
> > - should TabImport be fired before _swapBrowserDocShells?
> 
> (I have not tested yet, but) We can change it. However, it may not be useful
> when using TabImport event for other purpose. Firefox has not completed a
> swapping step before _swapBrowserDocShells.

Because tab progress listeners are unregistered/restored automatically before/after _swapBrowserDocShells, addons don't need events like "BeforeTabImport" for unregister/re-register custom progress listeners anymore, for progress listeners.

However, if possible, two events "BeforeTabImport/TabImport" (or "TabImport/TabImported") are still useful, because there can be rare problems which require unregsitering/re-registering something before/after _swapBrowserDocShells manually.

If only one new event type is acceptable, then, "before" event can work both before and after, with simple setTimeout hack.

Comment 25

[:Gavin Sharp [email: [e-mail]]]

OK, sounds like we should only have the before event. I think we should try to avoid initializing PopupNotifications in the new window if it is not initialized in the old window.

Comment 26

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Thank you for your comment, SHIMODA-san.

Gavin, I'll try to avoid initializing PopupNotifications like you said.
But my main business is very busy now. The next patch may be next year. (Sorry!)

Comment 27

[:Gavin Sharp [email: [e-mail]]]

No worries! I appreciate you working on this.

Comment 29

[Florian Quèze [:florian]]

Attachment: Patch v4 from bug 883326

Patch from bug 883326, moved here per Gavin's request. See bug 883326 for the context and discussion about previous iterations of this patch.

Comment 30

[:Gavin Sharp [email: [e-mail]]]

Bug 919835 ended up adding a "SwapDocShells" event.

Comment 31

[Florian Quèze [:florian]]

Attachment: Patch v4 from bug 883326 - unbitrotted

Comment 32

[Florian Quèze [:florian]]

Comment on attachment 8333899 [details] [diff] [review]
Patch v4 from bug 883326 - unbitrotted

I've just verified that this patch still applies and that the browser_popupNotification.js test still passes after the Australis landing.

Comment 33

[Florian Quèze [:florian]]

Comment on attachment 8333899 [details] [diff] [review]
Patch v4 from bug 883326 - unbitrotted

This patch has been waiting for months, so trying another reviewer.

Comment 34

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8333899 [details] [diff] [review]
Patch v4 from bug 883326 - unbitrotted

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

close, only two small changes and a question..

::: browser/modules/webrtcUI.jsm
@@ +134,5 @@
>      accessKey: stringBundle.getString("getUserMedia.shareSelectedDevices.accesskey"),
> +    // The real callback will be set during the "showing" event. The
> +    // empty function here is so that PopupNotifications.show doesn't
> +    // reject the action.
> +    callback: function() {}

was it necessary to move the "showing" code outside of this callback and into the eventCallback? if so, why?

::: toolkit/modules/PopupNotifications.jsm
@@ +769,5 @@
> +    let ourNotifications = this._getNotificationsForBrowser(ourBrowser);
> +    let other = otherBrowser.ownerDocument.defaultView.PopupNotifications;
> +    if (!other) {
> +      if (ourNotifications.length > 0)
> +        Components.utils.reportError("unable to swap notifications: otherBrowser doesn't support notifications");

nit: shorten it to Cu.reportError

@@ +780,5 @@
> +    }
> +
> +    otherNotifications = otherNotifications.filter(function (n) {
> +      if (n.options.eventCallback &&
> +          n.options.eventCallback.call(n, NOTIFICATION_EVENT_SWAPPING, ourBrowser)) {

it would be better if you also used _fireCallback here so that it can catch any errors thrown.  You could use the ...spread operator to pass the optional argument that only this notification uses.

@@ +796,5 @@
> +        n.browser = otherBrowser;
> +        n.owner = other;
> +        return true;
> +      }
> +      this._fireCallback(n, NOTIFICATION_EVENT_REMOVED);

is `this` correct here? I think it's not. use an arrow function to guarantee?

Comment 35

[Florian Quèze [:florian]]

Attachment: Patch v5

(In reply to :Felipe Gomes from comment #34)

Thanks for the feedback!

> ::: browser/modules/webrtcUI.jsm
> @@ +134,5 @@
> >      accessKey: stringBundle.getString("getUserMedia.shareSelectedDevices.accesskey"),
> > +    // The real callback will be set during the "showing" event. The
> > +    // empty function here is so that PopupNotifications.show doesn't
> > +    // reject the action.
> > +    callback: function() {}
> 
> was it necessary to move the "showing" code outside of this callback and
> into the eventCallback? if so, why?

Yes, so that 'chromeDoc' points to the correct chrome document, and not the chrome document where the notification was initially received.


> @@ +780,5 @@
> > +    }
> > +
> > +    otherNotifications = otherNotifications.filter(function (n) {
> > +      if (n.options.eventCallback &&
> > +          n.options.eventCallback.call(n, NOTIFICATION_EVENT_SWAPPING, ourBrowser)) {
> 
> it would be better if you also used _fireCallback here so that it can catch
> any errors thrown.

I think I hadn't done it because _fireCallback didn't return the value. But I guess it wouldn't hurt to make it return the value.

> You could use the ...spread operator to pass the
> optional argument that only this notification uses.

Done in this updated patch. It's the first time I use it, so please check carefully that its usage is correct here (I verified that the patch works after the changes).

> @@ +796,5 @@
> > +        n.browser = otherBrowser;
> > +        n.owner = other;
> > +        return true;
> > +      }
> > +      this._fireCallback(n, NOTIFICATION_EVENT_REMOVED);
> 
> is `this` correct here? I think it's not. use an arrow function to guarantee?

It was correct, because I gave 'this' as value for the second optional parameter of Array.filter. The new patch uses arrow function, as I guess they make it easier to see that 'this' is correct :).

Comment 36

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8350261 [details] [diff] [review]
Patch v5

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

::: toolkit/modules/PopupNotifications.jsm
@@ +812,3 @@
>      try {
>        if (n.options.eventCallback)
> +        return n.options.eventCallback.call(n, ...args);

yep, this is correct. But you can also keep the 'event' argument named, and only ...args for the remaining ones.

like

_fireCallback: function(n, event, ...args) {
   ...call(n, event, ...args)
}

Comment 37

[Florian Quèze [:florian]]

Attachment: Patch v5.1

Addressed comment 36.

Comment 38

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/b631a3f1a612

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b631a3f1a612

Comment 40

[Florian Quèze [:florian]]

Comment on attachment 810551 [details] [diff] [review]
Patch v4 from bug 883326

Not sure why bugzilla didn't remove this pending review flag when the attachment was marked obsolete.

Comment 41

[Ioana (away)]

Removing keyword since this is covered by automation.