Closed Bug 1365933 Opened 2 years ago Closed 2 years ago

Update and Restart loses window data, displays broken tabs (with "TypeError: tabData.entries[activeIndex] is undefined")

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 + fixed

People

(Reporter: Nika, Assigned: u462496)

References

Details

(Keywords: dataloss, dogfood, regression)

Attachments

(4 files, 2 obsolete files)

I clicked on the button to restart and update my browser. Instead of successfully restarting, the browser closed and did not restart. When I started it again, one of my windows did not restore correctly, instead having a couple of "New Tab" tabs, and a bunch of tabs which have no text on them. When they are clicked on they change their name to "New Tab" and render about:blank.

I opened the browser console,a nd noticed that the following error occured within SessionStore - I imagine that this was caused by failing to write out sessionstore when restarting? perhaps I had too many tabs open and writing out sessionstore was interrupted or similar?

We should definitely handle the case where sessionstore raises an exception while restoring a window better. At the very least this should instead show a page which says "sorry, we failed to restore your tabs"

A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Date: Thu May 18 2017 09:33:46 GMT-0400 (EDT)
Full Message: TypeError: tabData.entries[activeIndex] is undefined
Full Stack: ssi_restoreWindow@resource:///modules/sessionstore/SessionStore.jsm:3291:9
ssi_restoreWindows@resource:///modules/sessionstore/SessionStore.jsm:3468:5
initializeWindow@resource:///modules/sessionstore/SessionStore.jsm:1158:11
onBeforeBrowserWindowShown/<@resource:///modules/sessionstore/SessionStore.jsm:1307:9
process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:742:11
Probably related to bug 1360932.
Blocks: 1360932
Looks like a different issue to me.
(In reply to Tim Taubert [:ttaubert] from comment #2)
> Probably related to bug 1360932.

The window which failed to restore definitely did contain some lazy tabs from a previous restore before I restarted. I "save and quit" and restored again after this restore, and all of the blank tabs from the window disappeared when restoring for the second time.
I'm hitting this bug extremely reliably. I have a sessionstore.js file (generated from an unaffected Firefox version) that triggers it every time I start current Firefox.

I'll see if I can create a minimal sessionstore.js for testing / demonstration purposes. In the meantime, I strongly feel we should backout the changeset that introduced this regression (over on bug 1360932).  Commented/needinfo'd over there.

[Tracking Requested - why for this release]: regression causing dataloss
Has Regression Range: --- → yes
(In reply to Michael Layzell [:mystor] from comment #0)
> Full Message: TypeError: tabData.entries[activeIndex] is undefined
> Full Stack:
> ssi_restoreWindow@resource:///modules/sessionstore/SessionStore.jsm:3291:9

The line where we're throwing that ^^ error is the final line below, with the activeIndex dereference:
>      if (createLazyBrowser) {
>        // Let tabbrowser know the future URI because progress listeners won't
>        // get onLocationChange notification before the browser is inserted.
>        let activeIndex = (tabData.index || tabData.entries.length) - 1;
>        url = tabData.entries[activeIndex].url;
>      }
http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#3293


In my case, when the problem happens, activeIndex is -1!! And it's -1 because tabData.index is null, and tabData.entries.length is 0.  So we end up with activeIndex = 0 - 1 = -1, and then we try to use that -1 as an array index, which is invalid of course.
(In reply to Daniel Holbert [:dholbert] (reduced availability - travel & post-PTO backlog) from comment #7)
> In my case, when the problem happens, activeIndex is -1!! And it's -1
> because tabData.index is null, and tabData.entries.length is 0.  So we end
> up with activeIndex = 0 - 1 = -1, and then we try to use that -1 as an array
> index, which is invalid of course.

Well Mike, I guess this answers the question to https://bugzilla.mozilla.org/show_bug.cgi?id=1360932#c16 :-/

I'm working up a patch for this.
Flags: needinfo?(mdeboer)
Here's a sessionstore.js file that triggers what I think is this bug.  (At least, it definitely triggers the "tabData.entries[activeIndex] is undefined" issue and an incompletely-restored-session as a result.)

STR:
1. Place this attached sessionstore.js file in an empty folder.
2. Run Firefox Nightly using that folder as your firefox profile. For example, on linux:
  mkdir /tmp/foo;
  cp sessionstore.js /tmp/foo;
  /path/to/firefox -no-remote profile /tmp/foo

EXPECTED RESULTS:
I should end up with the following tabs:
   * CSS Box Alignment ( https://drafts.csswg.org/css-align/ ).
   * A "New Tab", which lazily resolves to example.org when you select it.
...and maybe some "first time/welcome" tabs.

ACTUAL RESULTS:
The expected tabs are missing. I only get the first time/welcome tabs.
(In reply to Kevin Jones from comment #8)
> Well Mike, I guess this answers the question to
> https://bugzilla.mozilla.org/show_bug.cgi?id=1360932#c16 :-/

To be extra clear -- it looks like the question in that comment was about whether tabData.index can be out of range.  AFAIK, this bug doesn't have that exact thing happening. Rather, in my case, tabData.index is simply null, and tabData.entries is *empty* (and has .length = 0).

> I'm working up a patch for this.

Awesome, thank you! Hopefully my just-attached sessionstore.js will be helpful in testing your patch locally.  I'm happy to give any patches a test run locally as well, if you'd like a sanity-check on "real" profile-data.
Summary: Update and Restart loses window data, displays broken tabs. → Update and Restart loses window data, displays broken tabs (with "TypeError: tabData.entries[activeIndex] is undefined")
(In reply to Kevin Jones from comment #8)
> Well Mike, I guess this answers the question to
> https://bugzilla.mozilla.org/show_bug.cgi?id=1360932#c16 :-/

Actually this is not exactly the same problem, since if tabData.entries.length == 0 then there will be no valid index, so simply mirroring "Ensure the index is in bounds." from ssi_restoreTab will not suffice.

A tab such as this gets its history updated from ssi_restoreTab with `activeIndex == 1` and `entries = []` curiously enough, but apparently this gets worked out in TabState or somewhere, as it doesn't seem to break, nor ever has.
This looks like a pretty serious regression, Mike, so if you can find time to review this soon I'm sure folks will appreciate it.  It is a pretty simple patch.

BTW, now that I'm paranoid, I went ahead and carried over the "out of bounds" check from restoreTab as well.
Assignee: nobody → kevinhowjones
Status: NEW → ASSIGNED
Attachment #8869870 - Flags: review?(mdeboer)
Feeling even more paranoid, and added a check for the existence of tabData.entries.
Attachment #8869871 - Flags: review?(mdeboer)
Attachment #8869870 - Attachment is obsolete: true
Attachment #8869870 - Flags: review?(mdeboer)
Mike, I messed up the order of things and it may look on your end I canceled the review, but #8869871 still needs one.  Thanks.
FWIW: I just tested attachment 8869871 [details] [diff] [review] locally and confirmed that it fixes the issue for me. (Thanks for the quick action, Kevin!)

I tried it:
 * with my minimal sessionstore.js testcase (attached to this bug)
 * with my much-larger from-my-actual-Firefox-profile sessionstore.js
...using STR from comment 9.  In both cases, m-c is broken without the patch but working fine with the patch.
Is it possible add a regression test for this?
Comment on attachment 8869871 [details] [diff] [review]
Don't attempt to get url when tabData.entries is non-existent or empty.

Review of attachment 8869871 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, Kevin!

::: browser/components/sessionstore/SessionStore.jsm
@@ +3290,5 @@
>          // get onLocationChange notification before the browser is inserted.
>          let activeIndex = (tabData.index || tabData.entries.length) - 1;
> +        // Ensure the index is in bounds.
> +        activeIndex = Math.min(activeIndex, tabData.entries.length - 1);
> +        activeIndex = Math.max(activeIndex, 0);

I guess I should've been on the fence after all. ;-)
Attachment #8869871 - Flags: review?(mdeboer) → review+
Flags: needinfo?(mdeboer)
Keywords: checkin-needed
(Looks like this patch needs a commit message before "checkin-needed" can be used.)
Flags: needinfo?(kevinhowjones)
Keywords: checkin-needed
And a test!
(In reply to Daniel Holbert [:dholbert] (reduced availability - travel & post-PTO backlog) from comment #18)
> (Looks like this patch needs a commit message before "checkin-needed" can be
> used.)

I'm not able to push this.  Mike, can you address this?
Flags: needinfo?(kevinhowjones) → needinfo?(mdeboer)
(In reply to Kevin Jones from comment #20)
> I'm not able to push this.  Mike, can you address this?

No, because Daniel and Ryan are right - can you at least upload a patch with a commit message (like you have done in the past on other bugs as well!)?
Do you have time to write a unit test? I think it'd be very useful to make sure we don't make the same mistake again somewhere in the future ;-)

However, you're free to do that in a follow-up, of course, to help people how are suffering from this bug on Nightly asap.
Flags: needinfo?(mdeboer) → needinfo?(kevinhowjones)
Session restore regression, tracking for 55.
(In reply to Mike de Boer [:mikedeboer] from comment #21)
> (In reply to Kevin Jones from comment #20)
> > I'm not able to push this.  Mike, can you address this?
> 
> No, because Daniel and Ryan are right - can you at least upload a patch with
> a commit message (like you have done in the past on other bugs as well!)?
> Do you have time to write a unit test? I think it'd be very useful to make
> sure we don't make the same mistake again somewhere in the future ;-)
> 
> However, you're free to do that in a follow-up, of course, to help people
> how are suffering from this bug on Nightly asap.

I'm sorry Mike, I'm not following you here.  I've have usually just uploaded patches just like I have here.  Sometimes I've created a commit message if I wanted to test something on the try server, but most times I haven't.  Maybe that was something Dao did afterwards for me?  Anyway, I am needing some direction here.
Flags: needinfo?(kevinhowjones) → needinfo?(mdeboer)
(In reply to Mike de Boer [:mikedeboer] from comment #21)
> (In reply to Kevin Jones from comment #20)
> > I'm not able to push this.  Mike, can you address this?
> 
> No, because Daniel and Ryan are right - can you at least upload a patch with
> a commit message (like you have done in the past on other bugs as well!)?
> Do you have time to write a unit test? I think it'd be very useful to make
> sure we don't make the same mistake again somewhere in the future ;-)
> 
> However, you're free to do that in a follow-up, of course, to help people
> how are suffering from this bug on Nightly asap.

And Yes, I'll work on a test in the meantime.  But I agree, it would be good to land this as soon as possible first.

Would the test just basically be restoring a session using the example session which exposed the bug, and testing that everything restored okay?

Also, should that be added to the existing test created in bug 1363078, or should it be a separate test?
(In reply to Kevin Jones from comment #23)
> I'm sorry Mike, I'm not following you here.  I've have usually just uploaded
> patches just like I have here.  Sometimes I've created a commit message if I
> wanted to test something on the try server, but most times I haven't.  Maybe
> that was something Dao did afterwards for me?  Anyway, I am needing some
> direction here.

I think we've been adding commit messages when landing patches for you. But generally, writing a (good) commit message is considered part of writing a patch. Reviewing that message is also part of the reviewers' responsibility.

I'll make a nice one up for you this time around, don't worry. It's something you'll want to consider for your future work.

(In reply to Kevin Jones from comment #24)
> Would the test just basically be restoring a session using the example
> session which exposed the bug, and testing that everything restored okay?

Yep!

> Also, should that be added to the existing test created in bug 1363078, or
> should it be a separate test?

Absolutely!
Flags: needinfo?(mdeboer)
(In reply to Mike de Boer [:mikedeboer] from comment #25)
> (In reply to Kevin Jones from comment #23)
> > I'm sorry Mike, I'm not following you here.  I've have usually just uploaded
> > patches just like I have here.  Sometimes I've created a commit message if I
> > wanted to test something on the try server, but most times I haven't.  Maybe
> > that was something Dao did afterwards for me?  Anyway, I am needing some
> > direction here.
> 
> I think we've been adding commit messages when landing patches for you. But
> generally, writing a (good) commit message is considered part of writing a
> patch. Reviewing that message is also part of the reviewers' responsibility.
> 
> I'll make a nice one up for you this time around, don't worry. It's
> something you'll want to consider for your future work.

I've been playing around with it, doing a local commit, then `hg diff -c tip`, but the patch still looks the same.  Maybe the commit message gets added manually afterwards?  If there is some documentation on this please point me to it.

Anyway, thanks for doing that.  I'll look at your patch and see what it should end up looking like.
 
> (In reply to Kevin Jones from comment #24)
> > Would the test just basically be restoring a session using the example
> > session which exposed the bug, and testing that everything restored okay?
> 
> Yep!
> 
> > Also, should that be added to the existing test created in bug 1363078, or
> > should it be a separate test?
> 
> Absolutely!

Absolutely what?  Add to the existing, or separate test?
Flags: needinfo?(mdeboer)
Use |hg export| instead of |hg diff| and your name/email/commit message will be there.
Patch with commit message
Attachment #8869871 - Attachment is obsolete: true
Attachment #8870045 - Flags: review?(mdeboer)
Comment on attachment 8870045 [details] [diff] [review]
1365933_patch_V3.diff

Review of attachment 8870045 [details] [diff] [review]:
-----------------------------------------------------------------

Cool! Thanks, Kevin.
Attachment #8870045 - Flags: review?(mdeboer) → review+
Flags: needinfo?(mdeboer)
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM] from comment #27)
> Use |hg export| instead of |hg diff| and your name/email/commit message will
> be there.

Thanks :-)
(In reply to Kevin Jones from comment #26)
> (In reply to Mike de Boer [:mikedeboer] from comment #25)
> > > Also, should that be added to the existing test created in bug 1363078, or
> > > should it be a separate test?
> > 
> > Absolutely!
> 
> Absolutely what?  Add to the existing, or separate test?

Er, still don't have an answer on this...
Flags: needinfo?(mdeboer)
(In reply to Kevin Jones from comment #31)
> Er, still don't have an answer on this...

Hrm, sorry. I meant 'Absolutely, feel free to add to the current test.'
Flags: needinfo?(mdeboer)
(In reply to Mike de Boer [:mikedeboer] from comment #32)
> (In reply to Kevin Jones from comment #31)
> > Er, still don't have an answer on this...
> 
> Hrm, sorry. I meant 'Absolutely, feel free to add to the current test.'

Okay, thanks :-)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5e45a082299e
Don't attempt to access tabData.entries value when tabData.entries is nonexistent or empty. r=mikedeboer
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5e45a082299e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Comment on attachment 8870081 [details] [diff] [review]
Unit test for tabData.entries nonexistent/empty regression

Review of attachment 8870081 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!
Attachment #8870081 - Flags: review?(mdeboer) → review+
Attachment #8870081 - Flags: checkin?(ryanvm)
Attachment #8870081 - Flags: checkin?(ryanvm)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3aad0cd65bd0
Unit test for tabData.entries nonexistent/empty regression. r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/3aad0cd65bd0
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.