584658 - Several actions in onLocationChange shouldn't be executed for location changes in frames

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Marco Bonardo [:mak]]

onLocationChange in tabbrowser.xml is called for both the main content but also for all subframes in the page.
Since we register an open page in it, we end up registering all sub frames as open.
Moreover, lastURI is updated with subframes location, we end up not unregistering the open tab in the destructor or any other code using lastURI.

This is the cause of the tabmatch failure I see in bug 552023 and most likely of the bugs where a tab is marked as open even if it's not.

Comment 1

[Marco Bonardo [:mak]]

bug 558626 could be related, i suspect zimbra has some frame.

Comment 2

[Marco Bonardo [:mak]]

ccing browser peers, since looks like onLocationChange has other stuff that could not want this behavior.

Comment 3

[Marco Bonardo [:mak]]

Attachment: patch v1.0

something like this, but needs a test.

Comment 4

[Dão Gottwald [:dao]]

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

>+              // OnLocationChange is called for both the main tab content
>+              // and the subframes.
>+              let isTabContent = aWebProgress.DOMWindow == this.mBrowser.contentWindow;

There's a term in DOM land for what you're calling main tab content: top-level window, as in window.top == window.

Also, no braces around single lines here.

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Dao, if you wish to steal review from Gavin, feel free to, I doubt he'll complain.

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 463149 [details] [diff] [review]
patch v1.1

>               if (this.mBrowser.userTypedClear > 0)
>                 this.mBrowser.userTypedValue = null;

Seems like we shouldn't reset userTypedValue for sub frame location changes either.

>               // changing location, clear out the missing plugins list
>               this.mBrowser.missingPlugins = null;

And this? Not sure what exactly this is doing though. The comment doesn't say more than the code itself...

>+                let browserHistory = this.mTabBrowser.mBrowserHistory;
>+                if ("lastURI" in this.mBrowser && this.mBrowser.lastURI)
>+                  browserHistory.unregisterOpenPage(this.mBrowser.lastURI);
>+                browserHistory.registerOpenPage(aLocation);

s/"lastURI" in this.mBrowser && //

Comment 7

[Marco Bonardo [:mak]]

(In reply to comment #6)
> Comment on attachment 463149 [details] [diff] [review]
> patch v1.1
> 
> >               if (this.mBrowser.userTypedClear > 0)
> >                 this.mBrowser.userTypedValue = null;
> 
> Seems like we shouldn't reset userTypedValue for sub frame location changes
> either.
> 
> >               // changing location, clear out the missing plugins list
> >               this.mBrowser.missingPlugins = null;
> 
> And this? Not sure what exactly this is doing though. The comment doesn't say
> more than the code itself...
> 

well, the subframes are supposed to load later, so these ends up being no-op. I can move them inside the condition but I've not done it because I don't know their interactions well enough.

Comment 8

[Dão Gottwald [:dao]]

(In reply to comment #7)
> > >               if (this.mBrowser.userTypedClear > 0)
> > >                 this.mBrowser.userTypedValue = null;

> > >               this.mBrowser.missingPlugins = null;

> well, the subframes are supposed to load later, so these ends up being no-op.

Depends on what populates missingPlugins and when, which I haven't researched. I also don't remember offhand if userTypedClear can change in a loaded document.

Comment 9

[Marco Bonardo [:mak]]

missingPlugins should be part of the plugin finder stuff. looks like the page annotates missing plugins here, and the notification bar for missing plugins uses it... So yeah the fact a subframe does not have missing plugins should probably not override the fact toplevel content is missing them.

Comment 10

[Marco Bonardo [:mak]]

userTypedClear looks like can be changed by back and forward buttons, that ideally can be pressed at any time during load or after it... But there are not many comments or docs about it.
At first glance looks like everything but the notifiation "rethrow" should be just for toplevel.

Cc-ing dolske for the plugin finder stuff

Comment 11

[Marco Bonardo [:mak]]

Attachment: patch v1.2

Can someone clarify the userTypedClear comment? "clear the value if we should" doesn't look like a really great comment.

Comment 12

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

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

Can't believe this has been broken like this. Can't look up history at the moment, but wonder whether we somehow regressed this...

Comment 13

[Justin Dolske [:Dolske]]

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

>+                // Clear out the missing plugins list since it's related to the
>+                // previous location.
>+                this.mBrowser.missingPlugins = null;

This is fine.

It does mean that if a subframe needed a plugin, and then changes to no longer need a plugin, the missingPlugins list is a bit stale. But I don't think it's worth trying to do ultra-precise tracking, and this seems better overall that what's presumably happening currently (subframe location changes clobbering the list for the whole tab).

Comment 14

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/4a063702e8c6