Closed Bug 584658 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 4.0b4
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: mak, Assigned: mak)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
bug 558626 could be related, i suspect zimbra has some frame.
blocking2.0: --- → ?
ccing browser peers, since looks like onLocationChange has other stuff that could not want this behavior.
Attached patch patch v1.0 (obsolete) — Splinter Review
something like this, but needs a test.
Flags: in-testsuite?
Blocks: 552023
Blocks: 558626
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.
Attached patch patch v1.1 (obsolete) — Splinter Review
Dao, if you wish to steal review from Gavin, feel free to, I doubt he'll complain.
Attachment #463120 - Attachment is obsolete: true
Attachment #463149 - Flags: review?(gavin.sharp)
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 && //
Blocks: 576605
(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.
(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.
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.
Component: Bookmarks & History → Tabbed Browser
QA Contact: bookmarks → tabbed.browser
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
Attached patch patch v1.2Splinter Review
Can someone clarify the userTypedClear comment? "clear the value if we should" doesn't look like a really great comment.
Attachment #463149 - Attachment is obsolete: true
Attachment #463161 - Flags: review?(gavin.sharp)
Attachment #463161 - Flags: feedback?(dolske)
Attachment #463149 - Flags: review?(gavin.sharp)
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...
Attachment #463161 - Flags: review?(gavin.sharp) → review+
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).
Attachment #463161 - Flags: feedback?(dolske) → feedback+
blocking2.0: ? → betaN+
http://hg.mozilla.org/mozilla-central/rev/4a063702e8c6
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b4
Flags: in-testsuite? → in-testsuite+
Summary: Switch to tab should not register subframes as open pages → Several actions in onLocationChange shouldn't be executed for location changes in frames
Duplicate of this bug: 587486
You need to log in before you can comment on or make changes to this bug.