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

Status: RESOLVED FIXED

(SeaMonkey :: Tabbed Browser, defect)

Comment 1

[Philip Chee]

From Bug 782033:
>>       if (aRequest && this.popupNotifications &&
>>           (aWebProgress.isLoadingDocument ||
>>-           !Components.isSuccessCode(aRequest.status)))
>>+           aFlags & nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE))
> [So, it turns out to be a little harder than that, in that aRequest can be null for an error page....]
> 
>>             onLocationChange: function(aWebProgress, aRequest, aLocation, aFlags) {
>>+              const nsIWebProgressListener = Components.interfaces.nsIWebProgressListener;
>>               if (aRequest && aWebProgress.DOMWindow == this.mBrowser.contentWindow) {
>>                 this.mBrowser.mIconURL = "";
>>                 this.mFeeds = [];
> [Here, things are trickier... dealing with null aRequest is going to need a follow-up bug.]
>

Comment 2

[[e-mail]]

>       if (aRequest && this.popupNotifications &&
>           (aWebProgress.isLoadingDocument ||
>-           !Components.isSuccessCode(aRequest.status)))
>+           aFlags & nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE))
Well, the obvious replacement is
if (this.popupNotifications &&
    (aRequest && aWebProgress.isLoadingDocument ||
     aFlags & nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE))
aRequest is null for a tab switch or some error pages.
aFlags is 0 for a tab switch or normal pages.
This way the code never fires for a tab switch because aRequest is null and aFlags is zero.
However, there is also a new LOCATION_CHANGE_SAME_DOCUMENT flag for an anchor scroll or a HTML5 history API call. So if we set aFlags to this value for a tab switch then we could simplify this to
if (this.popupNotifications &&
    !(aFlags & nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT))

>             onLocationChange: function(aWebProgress, aRequest, aLocation, aFlags) {
>+              const nsIWebProgressListener = Components.interfaces.nsIWebProgressListener;
>               if (aRequest && aWebProgress.DOMWindow == this.mBrowser.contentWindow) {
>                 this.mBrowser.mIconURL = "";
>                 this.mFeeds = [];
This probably needs to be
if (aWebProgress.DOMWindow == this.mBrowser.contentWindow &&
    !(aFlags & nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT))

However that doesn't work for the userTypedClear code which probably wants this:
if (aWebProgress.DOMWindow == this.mBrowser.contentWindow &&
    (this.mBrowser.userTypedClear > 0 ||
     aFlags & nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE))
  this.mBrowser.userTypedValue = null;

As for the rest of the code in that block it should probably die, it's there to load groupmarks which we don't support any more.

Comment 3

[Andrew McCreight [:mccr8]]

What sec rating do you think this should have, Philip?
https://wiki.mozilla.org/Security_Severity_Ratings

Comment 4

[[e-mail]]

Well, the original bug 623155 was sec-moderate. We implemented mitigation in the form of attachment 576901 [details] [diff] [review], although there are error pages for which that did not work. The work done in bug 782033 and here in this bug together should close that loophole.

Comment 5

[Philip Chee]

Attachment: Patch v1.0 Proposed fix.

>>       if (aRequest && this.popupNotifications &&
>>           (aWebProgress.isLoadingDocument ||
>>-           !Components.isSuccessCode(aRequest.status)))
>>+           aFlags & nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE))
> Well, the obvious replacement is
> if (this.popupNotifications &&
>     (aRequest && aWebProgress.isLoadingDocument ||
>      aFlags & nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE))
> aRequest is null for a tab switch or some error pages.
> aFlags is 0 for a tab switch or normal pages.
> This way the code never fires for a tab switch because aRequest is null and aFlags is zero.
> However, there is also a new LOCATION_CHANGE_SAME_DOCUMENT flag for an anchor scroll or a HTML5 history API call. So if we set aFlags to this value for a tab switch then we could simplify this to
> if (this.popupNotifications &&
>     !(aFlags & nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT))
Done.

>>             onLocationChange: function(aWebProgress, aRequest, aLocation, aFlags) {
>>+              const nsIWebProgressListener = Components.interfaces.nsIWebProgressListener;
>>               if (aRequest && aWebProgress.DOMWindow == this.mBrowser.contentWindow) {
>>                 this.mBrowser.mIconURL = "";
>>                 this.mFeeds = [];
> This probably needs to be
> if (aWebProgress.DOMWindow == this.mBrowser.contentWindow &&
>     !(aFlags & nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT))
OK.

> However that doesn't work for the userTypedClear code which probably wants this:
> if (aWebProgress.DOMWindow == this.mBrowser.contentWindow &&
>     (this.mBrowser.userTypedClear > 0 ||
>      aFlags & nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE))
>   this.mBrowser.userTypedValue = null;
OK.

> As for the rest of the code in that block it should probably die, it's there to load groupmarks which we don't support any more.
OK.

Comment 6

[Philip Chee]

Attachment: Patch v1.0 Proposed fix.

Oops wrong author again.

Comment 7

[[e-mail]]

Comment on attachment 652484 [details] [diff] [review]
Patch v1.0 Proposed fix.

With this patch, visiting wxyz:// now correctly:
* removes transient doorhangers
* removes transient notifications
* clears popup statusbar notification
* clears feed urlbar notification
* resets the urlbar to the error url

>-                if (this.mLocationChangeCount > 0 ||
>-                    aLocation.spec != "about:blank")
>-                  ++this.mLocationChangeCount;
>-
>-                if (this.mLocationChangeCount == 2) {
>-                  this.mTabBrowser.backBrowserGroup = [];
>-                  this.mTabBrowser.forwardBrowserGroup = [];
When I said "this should probably die" I meant the whole support for browser groups should be removed all at once in its own bug, rather than doing it piecemeal. For now, just put those lines back where they were. (Having thought about it a little more it's probably best that they don't check the flags at all anyway.) r=me with that fixed.

Comment 8

[[e-mail]]

Comment on attachment 652484 [details] [diff] [review]
Patch v1.0 Proposed fix.

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

Comment 9

[Philip Chee]

Attachment: Patch v1.1 more fixes r=Neil.

>>-                  this.mTabBrowser.forwardBrowserGroup = [];
> When I said "this should probably die" I meant the whole support for browser 
> groups should be removed all at once in its own bug, rather than doing it 
> piecemeal. For now, just put those lines back where they were. (Having thought 
> about it a little more it's probably best that they don't check the flags at all 
> anyway.) r=me with that fixed.

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

>> http://hg.mozilla.org/comm-central/rev/76ef05e53378
>>>>+                              tabListener.mStateFlags);
>>> State flags are not the same as location flags! Pass 0 for now. r=me with that fixed.
>> Fixed.
> Oops. Firstly, you passed the 0 in the wrong argument. (Partly my fault for 
> getting you to move it.) And also for bug 782516 I think you actually need to 
> pass LOCATION_CHANGE_SAME_DOCUMENT anyway.
Fixed.

Comment 10

[Philip Chee]

Comment on attachment 652794 [details] [diff] [review]
Patch v1.1 more fixes r=Neil.

Neil says "looks ok" over IRC.

Comment 11

[Philip Chee]

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