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)
Tracking
()
VERIFIED
FIXED
Firefox 61
People
(Reporter: Oriol, Assigned: Oriol)
References
Details
(Keywords: regression)
Attachments
(2 files)
59 bytes,
text/x-review-board-request
|
dao
:
review+
RyanVM
:
approval-mozilla-beta+
|
Details |
2.55 MB,
image/gif
|
Details |
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:")`
Comment 1•6 years ago
|
||
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)
Comment 2•6 years ago
|
||
Without clear other negative effects or STR, this shouldn't track for 58.
status-firefox57:
--- → wontfix
status-firefox58:
--- → fix-optional
status-firefox59:
--- → ?
Priority: -- → P3
Assignee | ||
Comment 3•6 years ago
|
||
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)
Comment 4•6 years ago
|
||
(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)
Assignee | ||
Comment 5•6 years ago
|
||
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
Assignee | ||
Comment 6•6 years ago
|
||
Effectively SessionStore.getLazyTabValue(gBrowser.tabs[0],"url") returns undefined even if the first tab is unloaded.
Comment 7•6 years ago
|
||
(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)
Assignee | ||
Comment 8•6 years ago
|
||
(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.
Assignee | ||
Comment 9•6 years ago
|
||
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)
Assignee | ||
Updated•6 years ago
|
Summary: "TypeError: url is undefined" when restoring session → "TypeError: url is undefined" when hovering prematurely inserted lazy tab
Comment 10•6 years ago
|
||
(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)
Assignee | ||
Comment 11•6 years ago
|
||
(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 hidden (mozreview-request) |
Comment 13•6 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 14•6 years ago
|
||
(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.
Comment 15•6 years ago
|
||
(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.
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(dao+bmo)
Comment 16•6 years ago
|
||
mozreview-review |
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) {
Comment 17•6 years ago
|
||
(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...
Comment 18•6 years ago
|
||
(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)
Comment 19•6 years ago
|
||
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.
Comment 20•6 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8959573 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 22•6 years ago
|
||
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)
Comment 23•6 years ago
|
||
(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 24•6 years ago
|
||
mozreview-review |
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+
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Updated•6 years ago
|
Assignee: nobody → oriol-bugzilla
Comment 25•6 years ago
|
||
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
Comment 26•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6d6728303386
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Comment 27•6 years ago
|
||
(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.
Updated•6 years ago
|
Comment 28•6 years ago
|
||
Is there a user impact here which warrants Beta uplift consideration?
Flags: needinfo?(oriol-bugzilla)
Assignee | ||
Comment 29•6 years ago
|
||
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 30•6 years ago
|
||
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+
Comment 31•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/17209d166650
Updated•6 years ago
|
Flags: qe-verify+
Comment 32•6 years ago
|
||
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)
Assignee | ||
Comment 33•6 years ago
|
||
(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)
Comment 34•6 years ago
|
||
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.
Description
•