Closed Bug 1309880 Opened 8 years ago Closed 8 years ago

Support undoing closeTab and closeWindow by ID

Categories

(Firefox :: Session Restore, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 52
Iteration:
52.3 - Nov 14
Tracking Status
firefox52 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

Details

Attachments

(1 file)

undoCloseTab and undoCloseWindow in SessionStore.jsm require an index to be passed into them that represents the current index of the tab or window in the SessionStore data. This creates an issue when one would like to query SessionStore and then later request that a window or tab be restored (because the index could have changed).

Our suggested solution to this is to create and store a unique ID for each closed tab and window in SessionStore, and then allow a consumer to query by ID to find the current index of the tab or window.

This would introduce a function into SessionStore.jsm along the lines of:

getUndoIndexById(id, type), where type is either window or tab

A consumer could then call, for example:

undoClosedWindow(getUndoIndexById(id, "window"))
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Comment on attachment 8800843 [details]
Bug 1309880 - Support undoing closeTab and closeWindow by ID,

I rebased my patch on top of central after bug 1309702 landed, and also fixed a problem with the test. This is ready for review.
Comment on attachment 8800843 [details]
Bug 1309880 - Support undoing closeTab and closeWindow by ID,

https://reviewboard.mozilla.org/r/85672/#review85502

This is f+, but we don't have that anymore with MozReview ;)

::: browser/components/sessionstore/SessionStore.jsm:4361
(Diff revision 2)
> +   * @param aWindow (optional)
> +   *        Window reference when searching for a tab
> +   *
> +   * @returns a number representing the index of the tab or window
> +   */
> +  getUndoIndexById(aClosedId, aClosedType, aWindow) {

What if we made the 'closedType' implicit?

```js
// This searches through the closed tabs of `someWindow`:
getUndoIndexById(3, someWindow);

// This searches for a closed window:
getUndoIndexById(2);
```

What do you think?

::: browser/components/sessionstore/SessionStore.jsm:4370
(Diff revision 2)
> +          return i;
> +        }
> +      }
> +    } else if (aClosedType == "tab") {
> +      if (!aWindow || !(aWindow instanceof Ci.nsIDOMChromeWindow)) {
> +        throw new Error(

Instead of throwing errors and returning `undefined`, I'd simply return `-1` when nothing is found. That (to me) is a good enough indication that the window or tab can not be found.

::: browser/components/sessionstore/SessionStore.jsm:4374
(Diff revision 2)
> +      if (!aWindow || !(aWindow instanceof Ci.nsIDOMChromeWindow)) {
> +        throw new Error(
> +          `SessionStore.getUndoIndexById must be passed a window of the type is "tab".`);
> +
> +      }
> +      for (let i = 0; i < this._windows[aWindow.__SSi]._closedTabs.length; i++) {

Please add a guard for when `this._windows[aWindow.__SSi]` is undefined.

::: browser/components/sessionstore/test/browser.ini:237
(Diff revision 2)
>  [browser_1234021.js]
>  [browser_remoteness_flip_on_restore.js]
>  run-if = e10s
>  [browser_background_tab_crash.js]
>  run-if = e10s && crashreporter
> +[browser_1309880.js]

We are now using the convention to name our test files in a way that the filename describes what the test does, not using the bug number.

Can you change that?
Attachment #8800843 - Flags: review?(mdeboer) → review-
Comment on attachment 8800843 [details]
Bug 1309880 - Support undoing closeTab and closeWindow by ID,

https://reviewboard.mozilla.org/r/85672/#review85502

> What if we made the 'closedType' implicit?
> 
> ```js
> // This searches through the closed tabs of `someWindow`:
> getUndoIndexById(3, someWindow);
> 
> // This searches for a closed window:
> getUndoIndexById(2);
> ```
> 
> What do you think?

Good idea! Fixed.
Comment on attachment 8800843 [details]
Bug 1309880 - Support undoing closeTab and closeWindow by ID,

https://reviewboard.mozilla.org/r/85672/#review85536

Alright, nicely contained, good test - ship it! (This needs a try-run before landing!)

::: browser/components/sessionstore/test/browser_getUndoIndexById.js:17
(Diff revision 4)
> +  yield TabStateFlusher.flush(tab.linkedBrowser);
> +  yield promiseRemoveTab(tab);
> +}
> +
> +add_task(function* test_getUndoIndexById() {
> +

nit: superfluous newline.

::: browser/components/sessionstore/test/browser_getUndoIndexById.js:35
(Diff revision 4)
> +  yield promiseBrowserLoaded(win.gBrowser.selectedBrowser);
> +
> +  // Open and close a tab.
> +  yield openAndCloseTab(win, "about:mozilla");
> +
> +  // Record the first closedId created

nit: missing dot.
Attachment #8800843 - Flags: review?(mdeboer) → review+
Comment on attachment 8800843 [details]
Bug 1309880 - Support undoing closeTab and closeWindow by ID,

https://reviewboard.mozilla.org/r/85672/#review85536

I changed this patch to implement undoCloseById instead, which turns out to be more useful and even simpler code. :)  Sorry for the initial version which wasn't really what was needed.
Comment on attachment 8800843 [details]
Bug 1309880 - Support undoing closeTab and closeWindow by ID,

Resetting the flag as the patch has changed.
Attachment #8800843 - Flags: review+ → review?(mdeboer)
Comment on attachment 8800843 [details]
Bug 1309880 - Support undoing closeTab and closeWindow by ID,

https://reviewboard.mozilla.org/r/85672/#review85554
Comment on attachment 8800843 [details]
Bug 1309880 - Support undoing closeTab and closeWindow by ID,

https://reviewboard.mozilla.org/r/85672/#review85558

r=me with comments addressed.

::: browser/components/sessionstore/SessionStore.jsm:2397
(Diff revision 5)
> +   *
> +   * @returns a tab or window object
> +   */
> +  undoCloseById(aClosedId) {
> +    // Check for a window first.
> +    for (let i = 0; i < this._closedWindows.length; i++) {

`for (let i = 0, l = this._closedWindows.length; i < l; ++i) {`

::: browser/components/sessionstore/SessionStore.jsm:2409
(Diff revision 5)
> +    let windowsEnum = Services.wm.getEnumerator("navigator:browser");
> +    while (windowsEnum.hasMoreElements()) {
> +      let window = windowsEnum.getNext();
> +      let windowState = this._windows[window.__SSi];
> +      if (windowState) {
> +        for (let i = 0; i < windowState._closedTabs.length; i++) {

`for (let j = 0, l = windowState._closedTabs.length; j < l; ++j) {`

::: browser/components/sessionstore/SessionStore.jsm:4382
(Diff revision 5)
>        for (let key of Object.keys(data.telemetry)) {
>          let histogram = Telemetry.getHistogramById(key);
>          histogram.add(data.telemetry[key]);
>        }
>      }
> -  }
> +  },

Why?

::: browser/components/sessionstore/test/browser_undoCloseById.js:4
(Diff revision 5)
> +"use strict";
> +
> +/**
> + * This test is for the getUndoIndexById helper function.

nit: please correct this comment.
Attachment #8800843 - Flags: review?(mdeboer) → review+
Comment on attachment 8800843 [details]
Bug 1309880 - Support undoing closeTab and closeWindow by ID,

https://reviewboard.mozilla.org/r/85672/#review85558

> Why?

Leftover from when getUndoIndexById was here.
Blocks: 1308060
Mike, this is failing on try [1]. I cannot reproduce it if I just run the one test, but if I run all the tests in browser/components/sessionstore/test then I am able to reproduce it consistently.

What's happening is that, on line 71 of the test, `undefined` is being passed as the value of `win` into the the call to BrowserTestUtils.waitForEvent(win, "load"). This is causing waitForEvent in BrowserTestUtils [2] to throw an exception because "subject is undefined". I'm not sure why `win` is undefined at this point in the test, when more than one test is run. Do you have any ideas? Is there something else I should be waiting for before calling waitForEvent?

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=99ade969ffca
[2] http://searchfox.org/mozilla-central/source/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#609
Flags: needinfo?(mdeboer)
I did some more investigation/debugging and here's what I found:

I can reliably reproduce the failure if I run `browser_parentProcessRestoreHash.js` before `browser_undoCloseById.js` although I have no idea why that is causing this result.

The problem is that when `_shouldSaveTabState` [1] is called from `maybeSaveClosedWindow` [2] after the second window is closed in the test (at line 67 of the test), the value of `aTabState.entries[0].url` is “about:blank”, whereas it should be “about:mozilla”. This is causing `shouldStore` to be “false” in `maybeSaveClosedWindow` which causes the window to not be stored in the `this._closedWindows` array. Therefore when the test tries to undoClosing that window (at line 70) it is not found which is why `undoCloseById` returns `undefined`.

If I add a delay between opening the window and closing it again then `aTabState.entries[0].url` is “about:mozilla” as expected, which allows the test to pass. I am surprised by this because I would have thought that the `yield promiseBrowserLoaded(win2.gBrowser.selectedBrowser);` at line 63 of the test would have been enough to ensure that the tab was loaded and the url would resolve to “about:mozilla”, but apparently not.

It’s also a bit of a mystery why this only happens after running `browser_parentProcessRestoreHash.js`. I am able to run most of the other tests in the folder before my test without issue, but if I run it after `browser_parentProcessRestoreHash.js` then this issue reproduces each time.

Is there something else I should be waiting for after opening the window to ensure the tab’s url has resolved appropriately? If so then maybe we don’t need to worry about why the other test is affecting this test.

Thanks again for your help with this, Mike. 

[1] http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#3952
[2] http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#1415
Thanks for investigating this, Bob!

With the following changes I get no failures anymore: https://pastebin.mozilla.org/8922493

Can you try it too? Thanks!
Flags: needinfo?(mdeboer) → needinfo?(bob.silverberg)
As an aside, I think more than one sessionstore test will be suffering from this behavior, perhaps causing intermittents. I'll file a bug to check the other tests if they'd benefit from more explicit `promiseBrowserLoaded` use.
Try looks good. Requesting check in.
Flags: needinfo?(bob.silverberg)
Keywords: checkin-needed
Target Milestone: --- → Firefox 52
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9f9d69a704e1
Support undoing closeTab and closeWindow by ID, r=mikedeboer
Keywords: checkin-needed
This is ready to re-land, thanks.
Flags: needinfo?(bob.silverberg)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/93fc611f0235
Support undoing closeTab and closeWindow by ID, r=mikedeboer
Keywords: checkin-needed
Depends on: 1313571
Depends on: 1313776
Sorry, backed out in https://hg.mozilla.org/integration/autoland/rev/3c1eddbe382f29d7f377426cf95756563a764b29. Not only would I have to file a third intermittent off this for https://treeherder.mozilla.org/logviewer.html#?job_id=5864808&repo=autoland but bug 1313776 is just too much to leave in.
No longer depends on: 1313776
Mike, I'm not sure what's going on with this latest failure. I looked over the test log, and all of the assertions are passing, but the test seems to take a very long time to start up. I've made a snippet of the log and posted it at https://gist.github.com/bobsilverberg/d380d83b897b7ab3e2ea09eef2bc34cc. It includes the full log from the test that runs before browser_undoCloseById.js (which is browser_switch_remoteness.js, ftr), as well as the full log of the run of browser_undoCloseById.js. Looking at this, I can see that browser_undoCloseById.js starts at 17:19:52, and then there is a log of debug output until 17:20:42 at which point the first test in the file appears to start running. The tests in the file end at 17:20:42, which suggests that the time to run the actual tests was less than 1000 ms, but the test is failing for taking too long. I don't know what's going on during all that debug output, nor what is causing it, but it seems like that's what we need to fix/address.

Could you please take a look and let me know what I might do to either fix this or investigate further.

This only seems to be a problem on the debug build.
Flags: needinfo?(mdeboer)
I'm going to add the code from bug 1312571, which implements SessionStore.lastClosedObjectType, to this bug's patch so this will duplicate bug 1312571.
Mike, I made the changes you suggested, and rolled in the fix that waits 20 ms after a window closes, but we're still seeing the timeout exceeded issue, and I think it looks the same. I created a new snippet for one of the new try runs, which can be found at [1].  You can see the latest version of the code at https://bugzilla.mozilla.org/attachment.cgi?id=8800843. It's intermittent, e.g., for my try push where I retriggered the suite a few times, I had 4 passes and 2 failures, and it only seems to happen on Linux debug.

Do you have any ideas of what I can do next?

[1] https://gist.github.com/bobsilverberg/493228bfc565530d140860881467188b
Well, disable the test on Linux Debug. I've done that before, but only as a last, throwing hands in the air, banging head on the table, resort.

Instead of pasting gists of the log, can you paste in the treeherder links? They are much more useful. Thanks!
Flags: needinfo?(mdeboer)
Thanks Mike. I will do that, resubmit to try, and trigger a bunch of runs to ensure that it cleans things up. My apologies for the log snippet. I will include links to Treeherder from now on. FTR, here's a link to the failure referenced above: https://treeherder.mozilla.org/#/jobs?repo=try&revision=18adb6ba4bfd&selectedJob=30188695.
Interestingly, I was just looking at another, completely unrelated intermittent, and it exhibits the exact same behaviour. So perhaps there is a different bug at play here, and for some reason we are just unlucky enough that it is popping up when we try to run this test. The related intermittent is at https://bugzilla.mozilla.org/show_bug.cgi?id=1308416, and I've asked Kris to take a look at it.

Considering this, do you have any further suggestions for where to go with this? We can still disable our test on linux debug, but I think there is a bigger issue here the perhaps shouldn't be ignored.
Flags: needinfo?(mdeboer)
See Also: → 1308416
I've retriggered a bunch more on all platforms for each suite 'browser/components/sessionstore/test/browser_undoCloseById.js' is in (which differs per platform, btw; bc1 on OSX debug != bc1 on Linux opt).

The bug you link to in comment 38 is about WebExtension API tests timing out intermittently, which might very well be the same issue as the one you're having with your own API that is a consumer of the code in this bug.
I don't think it will explain the intermittent failure of 'browser_undoCloseById.js', because it's in an entirely different component and suite.

I think disabling on Linux Debug is a valid option to consider here still. We'll see what Try/ Treeherder tells us in a bit.
Flags: needinfo?(mdeboer)
Thanks Mike. I realize that it is an entirely different component and suite, but I find it very interesting that we are seeing the exact same behaviour and only on Linux debug, which is why I suspect it's possible that they are both falling victim to a different bug which we are yet to uncover. The bug I linked to is for a WebExtension API test, but not one that is consuming this code. Yes it's completely unrelated, but very odd that the same inexplicable behaviour is being demonstrated. Just food for thought.
I say that the try run looks good, so I think you can try to disable the test on Linux Debug.
Thanks Mike. It's currently disabled in the attached patch, so I'll just request check in again.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3bee4763126e
Support undoing closeTab and closeWindow by ID, r=mikedeboer
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3bee4763126e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Apparently we decided to just knowingly merge this despite bug 1313776 still happening, so I'm reopening it.
You need to log in before you can comment on or make changes to this bug.