Closed Bug 1418009 Opened 6 years ago Closed 6 years ago

"TypeError: url is undefined" when hovering prematurely inserted lazy tab

Categories

(Firefox :: Session Restore, defect, P3)

56 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 61
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- verified
firefox61 --- verified

People

(Reporter: Oriol, Assigned: Oriol)

References

Details

(Keywords: regression)

Attachments

(2 files)

Lots of times, when I open Firefox (with automatic session restore), I see this error in the browser console:

    TypeError: url is undefined
    resource:///modules/sessionstore/SessionStore.jsm:3522:9
    resource:///modules/sessionstore/SessionStore.jsm:3542:22
    resource:///modules/sessionstore/SessionStore.jsm:345:5
    chrome://browser/content/tabbrowser.xml:8008:11
    chrome://browser/content/tabbrowser.xml:8159:9

Maybe it's because I have e10s disabled or something. Please check that `url` is not undefined before doing `url.startsWith("about:")`
Why the blocking bug - is this actually a regression from that bug? This is filed against 56 - which version of Firefox are you actually seeing this with? Any chance you can narrow down the steps to reproduce?

It's not clear to me off-hand that checking url is defined is the right thing - perhaps the real bug is that `url` *should* be defined... that's certainly what it looks like from the context.
Flags: needinfo?(oriol-bugzilla)
Without clear other negative effects or STR, this shouldn't track for 58.
Priority: -- → P3
I blocked bug 874533 because the line of code that throws the error was added in that bug.

https://searchfox.org/mozilla-central/rev/ba2b0cf4d16711d37d4bf4d267b187c9a27f6638/browser/components/sessionstore/SessionStore.jsm#3522

I'm getting this on latest Nightly 59 rev 709f355a7a8c4ae426d1824841a71ffdb5ce0137

Not sure if before bug 874533 there was another kind or error or not.
Flags: needinfo?(oriol-bugzilla)
(In reply to Oriol Brufau [:Oriol] from comment #3)
> I blocked bug 874533 because the line of code that throws the error was
> added in that bug.
> 
> https://searchfox.org/mozilla-central/rev/
> ba2b0cf4d16711d37d4bf4d267b187c9a27f6638/browser/components/sessionstore/
> SessionStore.jsm#3522
> 
> I'm getting this on latest Nightly 59 rev
> 709f355a7a8c4ae426d1824841a71ffdb5ce0137
> 
> Not sure if before bug 874533 there was another kind or error or not.

OK. It would be really helpful to have some steps to reproduce here. Is there any chance you can bisect the session restore file to figure out which tab is causing this? Or, if you're comfortable with it, send me a zipped/gzipped copy of the file (I wouldn't recommend attaching it to a public bug unless you verify there's no personal information in it at all) ?
Flags: needinfo?(oriol-bugzilla)
It seems this happens whenever I mouseover or mouseout an unloaded restored tab. Once I click it and it loads, then it's OK.

I bisected and I got https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=79cd0acfa99364d2bedd862959523d5869f8b8e9&tochange=cc352ddd68683ed0c59641cf82958c23d42adedf
Blocks: 1383073
No longer blocks: 874533
Flags: needinfo?(oriol-bugzilla)
Keywords: regression
Effectively SessionStore.getLazyTabValue(gBrowser.tabs[0],"url") returns undefined even if the first tab is unloaded.
(In reply to Oriol Brufau [:Oriol] from comment #6)
> Effectively SessionStore.getLazyTabValue(gBrowser.tabs[0],"url") returns
> undefined even if the first tab is unloaded.

I can't reproduce this. Do you have more detailed steps on how to get into this state from a clean profile? Is the data present in the session store file?

It sounds like the underlying bug may be that the data is not getting saved correctly (in some circumstances?) but without reproducing that issue, it's not clear to me how to proceed here...

(In reply to Oriol Brufau [:Oriol] from comment #5)
> I bisected and I got
> https://hg.mozilla.org/integration/autoland/
> pushloghtml?fromchange=79cd0acfa99364d2bedd862959523d5869f8b8e9&tochange=cc35
> 2ddd68683ed0c59641cf82958c23d42adedf

Is this just for the JS error showing up in the console, though? If the "url" value isn't there, that seems like it might be bad even besides just this error showing up in the console...
Flags: needinfo?(oriol-bugzilla)
(In reply to :Gijs from comment #7)
> I can't reproduce this. Do you have more detailed steps on how to get into
> this state from a clean profile? Is the data present in the session store
> file?

I can't reproduce in a clean profile neither, will try to find what configuration causes this.

> Is this just for the JS error showing up in the console, though?

Yes.
OK, steps to reproduce:

1. Load various pages in various tabs
2. Disable automatic session restore
3. Restart Firefox
4. Enter this code in the browser console:

    var myListener = {QueryInterface: XPCOMUtils.generateQI(
      ["nsIWebProgressListener", "nsISupportsWeakReference"]
    )};
    gBrowser.tabContainer.addEventListener("TabOpen",function(event){
      let browser = gBrowser.getBrowserForTab(event.target);
      browser.addProgressListener(myListener);
    }, false);

5. In hamburguer menu, restore previous session
6. Hover some restored tabs.

I guess the problem is that the code above causes this:
> Lazy browser prematurely inserted via 'addProgressListener' property access

Then SessionStore.getLazyTabValue no longer works.
Flags: needinfo?(oriol-bugzilla)
Summary: "TypeError: url is undefined" when restoring session → "TypeError: url is undefined" when hovering prematurely inserted lazy tab
(In reply to Oriol Brufau [:Oriol] from comment #9)
> OK, steps to reproduce:
> 
> 1. Load various pages in various tabs
> 2. Disable automatic session restore
> 3. Restart Firefox
> 4. Enter this code in the browser console:
> 
>     var myListener = {QueryInterface: XPCOMUtils.generateQI(
>       ["nsIWebProgressListener", "nsISupportsWeakReference"]
>     )};
>     gBrowser.tabContainer.addEventListener("TabOpen",function(event){
>       let browser = gBrowser.getBrowserForTab(event.target);
>       browser.addProgressListener(myListener);
>     }, false);


What's doing this normally (ie when not putting it into the browser console manually)? I'm assuming it's some add-on you're using?

I'm asking because we could try to shim addProgressListener() so that we don't create the browser but simply store the progress listeners until the browser *is* created (clearly, while the browser is lazy, there's no point listening for progress on it). That would be better generally, maybe.

However, if this is done on an add-on that's using webextensions, I also wonder if we can optimize the webextension's underlying APIs, whatever they are. If it's a legacy add-on, obviously that doesn't work...
Flags: needinfo?(oriol-bugzilla)
(In reply to :Gijs from comment #10)
> What's doing this normally (ie when not putting it into the browser console
> manually)? I'm assuming it's some add-on you're using?

This is done by a legacy add-on, X-notifier.

> I'm asking because we could try to shim addProgressListener() so that we
> don't create the browser but simply store the progress listeners until the
> browser *is* created (clearly, while the browser is lazy, there's no point
> listening for progress on it). That would be better generally, maybe.

Seems a good idea.
Flags: needinfo?(oriol-bugzilla)
Comment on attachment 8959573 [details]
Bug 1418009 - Avoid speculative connections on prematurely inserted lazy tabs

https://reviewboard.mozilla.org/r/228374/#review234270

This doesn't look like the right fix to me. Either the tab should have data or it shouldn't have all the other stuff that if statement is already checking (pending attribute, etc. etc.).
Attachment #8959573 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #13)
> This doesn't look like the right fix to me. Either the tab should have data

The data is removed when the browser is inserted. Should this change?
https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/browser/components/sessionstore/SessionStore.jsm#1908

> or it shouldn't have all the other stuff that if statement is already
> checking (pending attribute, etc. etc.).

The current conditions are not enough:
 - The pending attribute is there because the tab has not been restored.
 - _restore_on_demand just reflects a pref (sessionstore.restore_on_demand)
 - The __SS_connectionPrepared flag is only set inside the conditional, so when the condition is checked it's not set.
 - Not sure if your "etc. etc." includes anything else.

Note this issue does not require third-party add-ons, it can also be triggered by Firefox problems like bug 1445732.
(In reply to Oriol Brufau [:Oriol] from comment #14)
> (In reply to :Gijs from comment #13)
> > This doesn't look like the right fix to me. Either the tab should have data
> 
> The data is removed when the browser is inserted. Should this change?
> https://searchfox.org/mozilla-central/rev/
> 6e96a3f1e44e286ddae5fdafab737709741d237a/browser/components/sessionstore/
> SessionStore.jsm#1908

This doesn't make a lot of sense to me. AFAICT, if we hit that path, the browser has been inserted, and we then call `restoreTab`, which does:

    if (isBrowserInserted) {

(which is presumably true)

and then restores the tab (ie loads it, AIUI) in some cases. But not others...

> > or it shouldn't have all the other stuff that if statement is already
> > checking (pending attribute, etc. etc.).
> 
> The current conditions are not enough:
>  - The pending attribute is there because the tab has not been restored.
>  - _restore_on_demand just reflects a pref (sessionstore.restore_on_demand)
>  - The __SS_connectionPrepared flag is only set inside the conditional, so
> when the condition is checked it's not set.
>  - Not sure if your "etc. etc." includes anything else.
> 
> Note this issue does not require third-party add-ons, it can also be
> triggered by Firefox problems like bug 1445732.

I'm not really an expert on how lazy tabs and session restore interact. Mike would be a better bet, but he's on PTO. Dão would be my next guess. FWIW, from a quick look at the code, it should be using `getTabValue` instead of `getLazyTabValue` if the tab has had a browser inserted already. I don't really know why those two things are separate, or what is the 'right' way to check which one to use, or if there are other places in the code that have this problem, or if we should simply not be deleting this data until the tab has been restored, or if really it's a bug that we have 2 places to store the same data, and I don't have cycles to dig through sessionstore code to find out the answer to that question right now.
Flags: needinfo?(dao+bmo)
Comment on attachment 8959573 [details]
Bug 1418009 - Avoid speculative connections on prematurely inserted lazy tabs

https://reviewboard.mozilla.org/r/228374/#review234362

::: browser/components/sessionstore/SessionStore.jsm:3550
(Diff revision 1)
>     *
>     * @param tab
>     *        a tab to speculatively connect on mouse hover.
>     */
>    speculativeConnectOnTabHover(tab) {
> -    if (this._restore_on_demand && !tab.__SS_connectionPrepared && tab.hasAttribute("pending")) {
> +    if (this._restore_on_demand && tab.__SS_lazyData && !tab.__SS_connectionPrepared && tab.hasAttribute("pending")) {

I think we should strip this down to:

if (tab.__SS_lazyData && !tab.__SS_connectionPrepared) {
(In reply to Dão Gottwald [::dao] from comment #16)
> Comment on attachment 8959573 [details]
> Bug 1418009 - Avoid speculative connections on prematurely inserted lazy tabs
> 
> https://reviewboard.mozilla.org/r/228374/#review234362
> 
> ::: browser/components/sessionstore/SessionStore.jsm:3550
> (Diff revision 1)
> >     *
> >     * @param tab
> >     *        a tab to speculatively connect on mouse hover.
> >     */
> >    speculativeConnectOnTabHover(tab) {
> > -    if (this._restore_on_demand && !tab.__SS_connectionPrepared && tab.hasAttribute("pending")) {
> > +    if (this._restore_on_demand && tab.__SS_lazyData && !tab.__SS_connectionPrepared && tab.hasAttribute("pending")) {
> 
> I think we should strip this down to:
> 
> if (tab.__SS_lazyData && !tab.__SS_connectionPrepared) {

Shouldn't we also speculatively connect for browsers that have been inserted but not restored yet? AIUI they won't have __SS_lazyData...
(In reply to :Gijs from comment #17)
> (In reply to Dão Gottwald [::dao] from comment #16)
> > Comment on attachment 8959573 [details]
> > Bug 1418009 - Avoid speculative connections on prematurely inserted lazy tabs
> > 
> > https://reviewboard.mozilla.org/r/228374/#review234362
> > 
> > ::: browser/components/sessionstore/SessionStore.jsm:3550
> > (Diff revision 1)
> > >     *
> > >     * @param tab
> > >     *        a tab to speculatively connect on mouse hover.
> > >     */
> > >    speculativeConnectOnTabHover(tab) {
> > > -    if (this._restore_on_demand && !tab.__SS_connectionPrepared && tab.hasAttribute("pending")) {
> > > +    if (this._restore_on_demand && tab.__SS_lazyData && !tab.__SS_connectionPrepared && tab.hasAttribute("pending")) {
> > 
> > I think we should strip this down to:
> > 
> > if (tab.__SS_lazyData && !tab.__SS_connectionPrepared) {
> 
> Shouldn't we also speculatively connect for browsers that have been inserted
> but not restored yet?

We shouldn't normally hit that case. We consider it a bug when a browser gets prematurely inserted.
Flags: needinfo?(dao+bmo)
Another fix could be to use tab.linkedBrowser.currentURI.spec instead of this.getLazyTabValue(tab, "url"), but that's a bit stupid because in the normal case the currentURI getter would need to create an nsIURI on the fly.
(In reply to Dão Gottwald [::dao] from comment #19)
> Another fix could be to use tab.linkedBrowser.currentURI.spec instead of
> this.getLazyTabValue(tab, "url"), but that's a bit stupid because in the
> normal case the currentURI getter would need to create an nsIURI on the fly.

Then again prepareConnectionToHost will eventually need an nsIURI, so maybe we should change its signature to take one instead of a string.
Attachment #8959573 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8959573 [details]
Bug 1418009 - Avoid speculative connections on prematurely inserted lazy tabs

(In reply to Dão Gottwald [::dao] from comment #20)
> Then again prepareConnectionToHost will eventually need an nsIURI, so maybe
> we should change its signature to take one instead of a string.

I can also do this if you prefer it.
Attachment #8959573 - Flags: review?(dao+bmo)
(In reply to Oriol Brufau [:Oriol] from comment #22)
> Comment on attachment 8959573 [details]
> Bug 1418009 - Avoid speculative connections on prematurely inserted lazy tabs
> 
> (In reply to Dão Gottwald [::dao] from comment #20)
> > Then again prepareConnectionToHost will eventually need an nsIURI, so maybe
> > we should change its signature to take one instead of a string.
> 
> I can also do this if you prefer it.

I don't have a clear preference either way.
Comment on attachment 8959573 [details]
Bug 1418009 - Avoid speculative connections on prematurely inserted lazy tabs

https://reviewboard.mozilla.org/r/228374/#review234368
Attachment #8959573 - Flags: review?(dao+bmo) → review+
Keywords: checkin-needed
Assignee: nobody → oriol-bugzilla
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6d6728303386
Avoid speculative connections on prematurely inserted lazy tabs r=dao
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6d6728303386
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
(In reply to :Gijs from comment #15)
> from a quick look at the code, it should be using `getTabValue` instead of
> `getLazyTabValue` if the tab has had a browser inserted already. I don't
> really know why those two things are separate,

getTabValue and getLazyTabValue deal with entirely different kinds of data. I filed bug 1446719 on making this less misleading.
Is there a user impact here which warrants Beta uplift consideration?
Flags: needinfo?(oriol-bugzilla)
Comment on attachment 8959573 [details]
Bug 1418009 - Avoid speculative connections on prematurely inserted lazy tabs

Approval Request Comment

[Feature/Bug causing the regression]: bug 1383073

[User impact if declined]: the browser console is filled with lots of errors when hovering prematurely inserted lazy tabs. Probably this is not affecting lots of users, because normal browser usage should not generate them. But it may happen in some cases with webextensions (bug 1364847), or more easily if legacy extensions are enabled.

[Is this code covered by automated tests?]: No.

[Has the fix been verified in Nightly?]: I have manually verified in Nightly.

[Needs manual test from QE? If yes, steps to reproduce]: 
  1. Set devtools.chrome.enabled=true in about:config
  2. Make sure you have more than 1 tab, select one which is not the first one
  3. Restart Firefox and restore previous session
  4. Open the browser console (Ctrl+shift+J)
  5. Run gBrowser.tabs[0].linkedBrowser.addProgressListener
     Expected: Lazy browser prematurely inserted via 'addProgressListener' property access
  6. Hover the first tab with the mouse
     Expected: No "TypeError: url is undefined" error.

[List of other uplifts needed for the feature/fix]: none.

[Is the change risky?]: not risky.

[Why is the change risky/not risky?]: Some code uses getLazyTabValue to read data from __SS_lazyData. The patch checks that __SS_lazyData is defined before doing so.

[String changes made/needed]: none.
Flags: needinfo?(oriol-bugzilla)
Attachment #8959573 - Flags: approval-mozilla-beta?
Comment on attachment 8959573 [details]
Bug 1418009 - Avoid speculative connections on prematurely inserted lazy tabs

Fix spurious warnings in the browser console. Approved for 60.0b9.
Attachment #8959573 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
I have tried to reproduce this issue on Firefox 59.0a1 (2017-11-16) under Windows 10 x64 and Ubuntu 16.06 x32. I can see the "TypeError: url is undefined" error in browser console, but not when hover on the first tab, according to step 6 from comment 29. Moreover I wasn’t able to manually restore the previous session after restarting the browser (neither by changing settings inside the about:preferences page, nor by setting the preferences browser.sessionstore.max_tabs_undo and browser.sessionstore.max_windows_undo to 0). 

Is there something that I am missing here?
Flags: needinfo?(oriol-bugzilla)
Attached image screencast.gif
(In reply to Anca Soncutean [:Anca], Desktop Release QA from comment #32)
> I have tried to reproduce this issue on Firefox 59.0a1 (2017-11-16) under
> Windows 10 x64 and Ubuntu 16.06 x32. I can see the "TypeError: url is
> undefined" error in browser console, but not when hover on the first tab,
> according to step 6 from comment 29.

This screencast tests nightly (a3f183201f7f183c263d554bfb15fbf0b0ed2ea4) under Windows 10 x64. You can see that, when the mouse hovers the tab, errors keep appearing (the counter increases).

> Moreover I wasn’t able to manually
> restore the previous session after restarting the browser (neither by
> changing settings inside the about:preferences page, nor by setting the
> preferences browser.sessionstore.max_tabs_undo and
> browser.sessionstore.max_windows_undo to 0). 

Not sure why you weren't able. In this screencast I restarted using

  var a = Components.interfaces.nsIAppStartup;
  Components.classes["@mozilla.org/toolkit/app-startup;1"]
  .getService(a).quit(a.eRestart | a.eAttemptQuit);

this seems to restore previous session even if about:preferences says "Show your home page" at startup. But other approaches should work too.
Flags: needinfo?(oriol-bugzilla)
I was able to reproduce this issue on Firefox 59.0a1 (2017-11-16) under Windows 10 x64 and Ubuntu 16.04 x32 with the additional information, mentioned in comment 33. 
I retested everything on Firefox 61.0a1 (2018-04-03) and Firefox 60.0b9 (20180402175344) under Windows 10 x64, Ubuntu 16.04 x32 and macOS 10.13 and the "TypeError: url is undefined" error is not displayed anymore. Therefore I will mark this bug as verified fix.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: