959536 - Session restore causes browser's onLocationChange (and related code) to fire repeatedly for the same URL, intermingled with about:blank, for restored tabs

Status: NEW

(Firefox :: Session Restore, defect)

Description

[:Gijs (he/him)]

So XULBrowserWindow.onLocationChange in browser.js (which tracks the currently open page in the tabbrowser, and updates button states etc. for this purpose) gets called a lot on startup.

Two walkthroughs of what happens:

If you have a "New Tab" page open, close the browser, and reopen it, and that tab gets restored as part of startup as the selected tab, just for startup, we go through onLocationChange 5 times in the following order:

- about:home (or your homepage) for the default tab which is in the tabbrowser before session history restores the browser
- about:blank for the new tab which gets selected by the session restore code and which will cause an onselect event on the tabbrowser, which ends up calling onLocationChange
- about:newtab because session store in the restoreTabs loop sets currentURI for the browser in order for url/title tab search to work
- about:blank (again) because session store *after* the restoreTabs loop immediately calls restoreNextHistory->restoreHistory->restoreTabContent, the latter of which starts with setting the tab to about:blank again because otherwise the docshell gets confused (so says the comment)
- about:newtab (again) because then session store in restoreTabContent actually loads the URI by asking it to get the entry at the respective index and reload it (which in effect will be the first time it's *actually* loaded, I *think*).

Then if you select a tab with URL foo.com, we do:

- foo.com for the selected tab and its URI
- about:blank because restoreTabContent then resets the URI back to about:blank to un-confuse the docshell (see above)
- foo.com for the load of that tab.


This seems pretty inefficient, and can also cause confusion in code that does async stuff from onLocationChange (more or less directly).

Tim/Steven, do you see a way to optimize this? I know that there are bugs filed on the homepage being loaded initially even in the session restore case; that would help a little, but even then I think we should try to think about minimizing the impact here, because the overhead starts adding up.

On the other hand, I guess that stuff should be in the right state when we start loading foo.com but haven't finished yet, and about:blank is in fact the thing that's in the docshell... I'm just suspicious that the current way of doing isn't the most efficient and we could optimize this. :-)

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gijs Kruitbosch from comment #0)
> - about:newtab because session store in the restoreTabs loop sets currentURI
> for the browser in order for url/title tab search to work
> - about:blank (again) because session store *after* the restoreTabs loop
> immediately calls restoreNextHistory->restoreHistory->restoreTabContent, the
> latter of which starts with setting the tab to about:blank again because
> otherwise the docshell gets confused (so says the comment)
> - about:newtab (again) because then session store in restoreTabContent

This was implemented in bug 599909 and that has quite a few comments on why this is needed. It does seem indeed awkward but I assume that a lot of code nowadays depends on browser.currentURI, especially since we restore tabs on demand by default.

> On the other hand, I guess that stuff should be in the right state when we
> start loading foo.com but haven't finished yet, and about:blank is in fact
> the thing that's in the docshell... I'm just suspicious that the current way
> of doing isn't the most efficient and we could optimize this. :-)

Yeah it seems like there should be some optimizations that can be done here. switchToTabHavingURI() really doesn't need us triggering onLocationChange() events anymore, it however does access browser.currentURI.

Also for tabs that are immediately restored, like the selected tab and two or three more per pref defaults, we could totally skip the setCurrentURI() step because we know they're loaded right away.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #1)
> switchToTabHavingURI() really doesn't need us triggering onLocationChange()
> events anymore, it however does access browser.currentURI.

Nah, I was wrong. We totally need those onLocationChange() notifications.

Comment 3

[:Gijs (he/him)]

(In reply to Tim Taubert [:ttaubert] from comment #2)
> (In reply to Tim Taubert [:ttaubert] from comment #1)
> > switchToTabHavingURI() really doesn't need us triggering onLocationChange()
> > events anymore, it however does access browser.currentURI.
> 
> Nah, I was wrong. We totally need those onLocationChange() notifications.

Can we eliminate the initial change onLocationChange that gets currentURI instead of about:blank, which is directly followed by the about:blank load? I see no reason why we could need that - it isn't the URL that's actually loaded.

More broadly, I guess if we restore tabs we shouldn't really even load the homepage - we should just restore all the tab urls, titles and favicons in one go, and dynamically create about:blank docshells as needed... but that's probably harder.

Comment 4

[:Gijs (he/him)]

(In reply to Tim Taubert [:ttaubert] from comment #1)
> Also for tabs that are immediately restored, like the selected tab and two
> or three more per pref defaults, we could totally skip the setCurrentURI()
> step because we know they're loaded right away.

Well, and that.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-959536-Don-t-use-setCurrentURI-to-register-pendi.patch

Bug 599909 introduced using webNavigation.setCurrentURI() to register pending tabs with mozIPlacesAutoComplete because it sends an onLocationChange() notification.

I think we should rather explicitly register the "open" URL with mozIPlacesAutoComplete instead of going the onLocationChange() way as that has lots of side-effects, as reported here. It also doesn't make us lie anymore about the actual state of the docShell wrt .currentURI.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-959536-Don-t-use-setCurrentURI-to-register-pendi.patch, v2

That also means we need to teach switchToTabHavingURI() to first check .registeredOpenURI and then .currentURI if the former property is null/doesn't exist.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-959536-Don-t-use-setCurrentURI-to-register-pendi.patch, v3

Turns out there is some Panorama code relying on .currentURI. It knows about pending tabs so it should also check .registeredOpenURI.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8364307 [details] [diff] [review]
0001-Bug-959536-Don-t-use-setCurrentURI-to-register-pendi.patch, v3

'browser.registeredOpenURI || browser.currentURI' doesn't seem like an intuitive API that I'd want our and third-party code to use at a large scale, assuming the number of affected add-ons was even manageable in the first place. I actually suspect that there are too many add-ons relying on currentURI, though.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

So... how else could we make switch-to-tab work without sending all the onLocationChange() notifications and confusing the docShell? Would a "temporary" override for .currentURI on the <browser> be a thing we could do? That could always return a given URI until it's told otherwise?

Comment 10

[:Gijs (he/him)]

(In reply to Tim Taubert [:ttaubert] from comment #9)
> So... how else could we make switch-to-tab work without sending all the
> onLocationChange() notifications and confusing the docShell? Would a
> "temporary" override for .currentURI on the <browser> be a thing we could
> do? That could always return a given URI until it's told otherwise?

Could we abstract the logical or into the currentURI getter, so no consumers need to change? Or does onLocationChange depend directly on the value of currrentURI changing?