1035171 - Switching tabs triggers onLocationChange with LOCATION_CHANGE_SAME_DOCUMENT flag

Status: RESOLVED FIXED

Description

[Wladimir Palant]

SeaMonkey's implementation of web progress listeners for the tabbrowser widget behaves differently from the one in Firefox. When the user switches to another tab both implementations will trigger onLocationChange listeners. However, Firefox calls it without any flags whereas SeaMonkey will always use nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT flag. This makes no sense whatsoever since the new URLs clearly belongs to a different document. This issue was apparently introduced in bug 782516 but I cannot mark this bug as blocking it - bug 782516 is still marked as sensitive for some reason.

Details: tabbrowser.updateCurrentBrowser() is called when switching to another tab. It calls tabbrowser.updateUrlBar() with nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT as aFlags parameter. And tabbrowser.updateUrlBar() will in turn forward that parameter to the onLocationBar method in web progress listeners.

Comment 1

[[e-mail]]

(In reply to bug 782516 comment #8)
> >+      if (this.popupNotifications &&
> >+          !(aFlags & nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT))
> >         this.popupNotifications.locationChange();
> Oops, this doesn't quite work, because it triggers when you switch tabs. A
> bit of a hacky workaround is to get the tab-switching code to send the
> LOCATION_CHANGE_SAME_DOCUMENT flag in this case.

This code was then removed in bug 843800, so the hack is no longer necessary.

Comment 2

[Philip Chee]

Hi Wladimir! Is there anything else to this bug? Otherwise I'm going to close this.

Comment 3

[Wladimir Palant]

Why close? The way I understand comment 1 nothing prevents fixing this bug. As things are now, Adblock Plus functionality is broken because SeaMonkey is lying to it - and I see no way to work around this.

Comment 4

[Philip Chee]

(In reply to Wladimir Palant from comment #3)
> Why close? The way I understand comment 1 nothing prevents fixing this bug.
> As things are now, Adblock Plus functionality is broken because SeaMonkey is
> lying to it - and I see no way to work around this.

Somebody who understands this code should submit a patch to remove the hack.

> This code was then removed in bug 843800, so the hack is no longer necessary.

Comment 5

[Philip Chee]

Attachment: Patch v1.0 don't send LOCATION_CHANGE_SAME_DOCUMENT

> This code was then removed in bug 843800, so the hack is no longer necessary.

Comment 6

[Philip Chee]

comm-central trees are closed at the moment. But will push when it reopens.

Comment 7

[Philip Chee]

Comment on attachment 8466986 [details] [diff] [review]
Patch v1.0 don't send LOCATION_CHANGE_SAME_DOCUMENT

[Approval Request Comment]
Regression caused by (bug #): Bug 782516 [1]
User impact if declined: AdblockPlus can't function properly. Probably also affects other content blocker extensions.
Testing completed (on m-c, etc.): been cooking in my local tree for a month.
Risk to taking this patch (and alternatives if risky): very low risk regression fix.
String changes made by this patch: None.

[1] Bug 782516 [tabbrowser.xml] onLocationChange: aRequest can be null for an error page.

Comment 8

[Philip Chee]

Pushed to comm-central: http://hg.mozilla.org/comm-central/rev/e78fb70d9419

Pushed to branches:
https://hg.mozilla.org/releases/comm-aurora/rev/41c5afb2089e
https://hg.mozilla.org/releases/comm-beta/rev/ec9d0275b763

Pushed to comm-release (2.29.1)
https://hg.mozilla.org/releases/comm-beta/rev/ec9d0275b763