Switching tabs triggers onLocationChange with LOCATION_CHANGE_SAME_DOCUMENT flag

RESOLVED FIXED in seamonkey2.32

Status

SeaMonkey
Tabbed Browser
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Wladimir Palant (for Adblock Plus info Cc bugzilla@adblockplus.org), Assigned: Philip Chee)

Tracking

(Blocks: 1 bug)

Trunk
seamonkey2.32

SeaMonkey Tracking Flags

(seamonkey2.26 wontfix, seamonkey2.27 wontfix, seamonkey2.28 affected, seamonkey2.29 fixed, seamonkey2.30 fixed, seamonkey2.31 fixed, seamonkey2.32 fixed)

Details

Attachments

(1 attachment)

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

4 years ago
(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.
(Assignee)

Comment 2

4 years ago
Hi Wladimir! Is there anything else to this bug? Otherwise I'm going to close this.
Flags: needinfo?(trev.moz)
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.
Flags: needinfo?(trev.moz)
(Assignee)

Comment 4

4 years ago
(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.
(Assignee)

Comment 5

4 years ago
Created attachment 8466986 [details] [diff] [review]
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.
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Attachment #8466986 - Flags: review?(neil)

Updated

4 years ago
Attachment #8466986 - Flags: review?(neil) → review+
(Assignee)

Comment 6

4 years ago
comm-central trees are closed at the moment. But will push when it reopens.
status-seamonkey2.26: --- → wontfix
status-seamonkey2.27: --- → wontfix
status-seamonkey2.28: --- → affected
status-seamonkey2.29: --- → affected
status-seamonkey2.30: --- → affected
status-seamonkey2.31: --- → affected
(Assignee)

Comment 7

3 years ago
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.
Attachment #8466986 - Flags: approval-comm-release?
Attachment #8466986 - Flags: approval-comm-beta?
Attachment #8466986 - Flags: approval-comm-aurora?

Updated

3 years ago
Attachment #8466986 - Flags: approval-comm-release?
Attachment #8466986 - Flags: approval-comm-release+
Attachment #8466986 - Flags: approval-comm-beta?
Attachment #8466986 - Flags: approval-comm-beta+
Attachment #8466986 - Flags: approval-comm-aurora?
Attachment #8466986 - Flags: approval-comm-aurora+
(Assignee)

Comment 8

3 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-seamonkey2.29: affected → fixed
status-seamonkey2.30: affected → fixed
status-seamonkey2.31: affected → fixed
status-seamonkey2.32: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.32
You need to log in before you can comment on or make changes to this bug.