1334266 - Implement browser.sessions.forgetClosedTab and forgetClosedWindow

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P5)

Description

[Kain]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170123004004

Steps to reproduce:

I attempted to update my XPCOM extension (https://addons.mozilla.org/en-US/firefox/addon/historyblock/) to Web Extension and found some of the core APIs missing.


Actual results:

Specifically, HistoryBlock relies on `nsIBrowserHistory.removePagesFromHost` as well as `nsISessionStore.forgetClosedTab` and `nsISessionStore.forgetClosedWindow` (currently, only the former, but I would like both).


Expected results:

It would be extremely helpful if the `sessions` Web Extension API included `forgetClosedTab` and `forgetClosedWindow`, and if the `history` Web Extension API included `removePagesFromHost` (though, I can probably get by with the existing `deleteUrl`... maybe).

Comment 1

[Kain]

For some context on HistoryBlock (https://addons.mozilla.org/en-US/firefox/addon/historyblock/):

HistoryBlock allows users to store hashed hostnames in a blacklist, and when a user visits a blacklisted website the history entries are immediately purged. Additionally, if a tab is closed (and hopefully in an upcoming version, if a window is closed) containing a blacklisted hostname, then it is purged from the 'Recently Closed Tabs/Windows' list.

Additionally, cache entries appear to also be purged, though there is no API call for that other than via the history manager's `removePagesFromHost`.

Comment 2

[Kain]

I have created an experimental implementation of the two session APIs I require: https://github.com/kainsavage/moresessions

Comment 3

[Kain]

Additionally, after some experimentation, I do not require `removePagesFromHost` as `browser.history.deleteUrl` is sufficient.

Comment 4

[Caitlin Neiman [:caitmuenster]]

This has been added to the March 7 WebExtensions Triage meeting agenda. 

Agenda: https://docs.google.com/document/d/1zzfedbTKAHUmm4UctxSOIJV3iKayXQ8TuXPodXW8wC0/edit#

Comment 5

[Bob Silverberg [:bsilverberg]]

Kain, this proposal has been approved, and we think it makes sense to implement these functions directly in the sessions API, rather than as an experiment. Is this something you are interested in working on?

Comment 6

[Kain]

Bob, I would love to work on this directly, but my schedule has gotten extremely busy of late (notice, I have not even submitted the pull request for my experiment yet and I said I would do that two weeks ago... apologies).

If there is no one else willing/able to work on it, I would happily donate some time as it becomes available to me, but I do not know when that will be.

Comment 7

[Bob Silverberg [:bsilverberg]]

Thanks for the update, Kain. I will unassign you in case someone else comes along who wants to work on it, but when you do find the time please feel free to pick it up where you left off.

Comment 8

[Thomas Wisniewski]

Attachment: 1334266-add_WebExtension_support_for_forgetClosedTab_and_Window.diff

Will this do?

Comment 9

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8853629 [details] [diff] [review]
1334266-add_WebExtension_support_for_forgetClosedTab_and_Window.diff

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

Thanks Thomas, this is really great work and a very good start.

I have added comments to a number of the files, most of which are fairly minor things. I think the two tests need to be changed significantly and I have explained what needs to be done in a comment on `browser_ext_sessions_forgetClosedWindow.js`.

Re-flag me for review after you've had a chance to make the changes, and let me know if you have any questions about any of my comments.

::: browser/components/extensions/ext-sessions.js
@@ +63,5 @@
>          return Promise.resolve(getRecentlyClosed(maxResults, extension));
>        },
>  
> +      forgetClosedTab: function(windowId, sessionId) {
> +        let aWindow = context.extension.windowManager.get(windowId).window;

Please use `window` instead of `aWindow`. We don't use prefixes in WebExtensions code.

@@ +66,5 @@
> +      forgetClosedTab: function(windowId, sessionId) {
> +        let aWindow = context.extension.windowManager.get(windowId).window;
> +        let closedTabData = SessionStore.getClosedTabData(aWindow, false);
> +
> +        sessionId = parseInt(sessionId);

You could inline this on line 73, rather than adding this extra statement.

@@ +68,5 @@
> +        let closedTabData = SessionStore.getClosedTabData(aWindow, false);
> +
> +        sessionId = parseInt(sessionId);
> +
> +        let closedTabIndex = closedTabData.findIndex((aClosedTab) => {

aClosedTab should be closedTab.

@@ +83,5 @@
> +
> +      forgetClosedWindow: function(sessionId) {
> +        let closedWindowData = SessionStore.getClosedWindowData(false);
> +
> +        sessionId = parseInt(sessionId);

As above, this can be inlined on line 90.

@@ +85,5 @@
> +        let closedWindowData = SessionStore.getClosedWindowData(false);
> +
> +        sessionId = parseInt(sessionId);
> +
> +        let closedWindowIndex = closedWindowData.findIndex((aClosedWindow) => {

aClosedWindow should be closedWindow.

::: browser/components/extensions/schemas/sessions.json
@@ +57,5 @@
>      "functions": [
>        {
> +        "name": "forgetClosedTab",
> +        "type": "function",
> +        "description": "Attempts to forget a recently closed tab.",

I think it would be better to remove the "Attempts to" part of this description, and just say something like "Forget a recently closed tab.".

@@ +63,5 @@
> +        "parameters": [
> +          {
> +            "name": "windowId",
> +            "type": "integer",
> +            "description": "The window to which the recently closed tab to be forgotten belongs."

Should be "the windowId of the window...".

@@ +71,5 @@
> +            "type": "string",
> +            "description": "The sessionId (closedId) of the recently closed tab to be forgotten."
> +          },
> +          {
> +            "type": "function",

This parameter should be omitted as this is an async: true method, and therefore will not work with a callback.

@@ +81,5 @@
> +      },
> +      {
> +        "name": "forgetClosedWindow",
> +        "type": "function",
> +        "description": "Attempts to forget a recently closed window.",

I think it would be better to remove the "Attempts to" part of this description, and just say something like "Forget a recently closed window.".

@@ +90,5 @@
> +            "type": "string",
> +            "description": "The sessionId (closedId) of the recently closed window to be forgotten."
> +          },
> +          {
> +            "type": "function",

This parameter should be omitted as this is an async: true method, and therefore will not work with a callback.

::: browser/components/extensions/test/browser/browser_ext_sessions_forgetClosedTab.js
@@ +1,1 @@
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */

Please look at my comments on `browser_ext_sessions_forgetClosedWindow.js` as most of them apply here as well. Much can be removed from this test and it can be written to simply check what needs to be checked. I.e., open and close a couple of tabs, forget one of them, and make sure that the forgotten tab is no longer in the data from getRecentlyClosed.

::: browser/components/extensions/test/browser/browser_ext_sessions_forgetClosedWindow.js
@@ +1,5 @@
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set sts=2 sw=2 et tw=80: */
> +"use strict";
> +
> +/* globals recordInitialTimestamps, onlyNewItemsFilter, checkRecentlyClosed */

None of these should be needed. `recordInitialTimestamps` isn't used, and the use of the others is unnecessary because this test doesn't need to worry about all of the data that is returned by `browser.sessions.getRecentlyClosed`.

@@ +3,5 @@
> +"use strict";
> +
> +/* globals recordInitialTimestamps, onlyNewItemsFilter, checkRecentlyClosed */
> +
> +requestLongerTimeout(2);

This should be removed. It was required by a specific test, but this test does not need it.

@@ +5,5 @@
> +/* globals recordInitialTimestamps, onlyNewItemsFilter, checkRecentlyClosed */
> +
> +requestLongerTimeout(2);
> +
> +Services.scriptloader.loadSubScript(new URL("head_sessions.js", gTestPath).href,

As we will not be using any of the functions exported from `head_sessions.js`, this isn't needed.

@@ +9,5 @@
> +Services.scriptloader.loadSubScript(new URL("head_sessions.js", gTestPath).href,
> +                                    this);
> +
> +add_task(function* test_sessions_forget_closed_window() {
> +  function* openAndCloseWindow(url = "http://example.com", tabUrls) {

This test never checks the contents of a closed window, so it's unnecessary to complicate it by opening windows with varying numbers of tabs. This function could be either removed, or simplified to just open and close a window for a single URL.

@@ +22,5 @@
> +    yield BrowserTestUtils.closeWindow(win);
> +  }
> +
> +  function background() {
> +    browser.tabs.query({active: true, currentWindow: true}).then(tabs => {

Because this test is only concerned with windows and not tabs, we don't need to know this `currentWindowId` so this whole section can be removed.

@@ +26,5 @@
> +    browser.tabs.query({active: true, currentWindow: true}).then(tabs => {
> +      browser.test.sendMessage("initialData", {currentWindowId: tabs[0].windowId});
> +    });
> +
> +    browser.test.onMessage.addListener((msg, filter) => {

`filter` is never used, so just remove it from the background code.

@@ +31,5 @@
> +      if (msg === "check-sessions") {
> +        browser.sessions.getRecentlyClosed(filter).then(recentlyClosed => {
> +          browser.test.sendMessage("recentlyClosed", recentlyClosed);
> +        });
> +      } else if (msg.forget) {

This is inconsistent and confusing. Just use a message named "forget" and pass the sessionId as an argument.

@@ +34,5 @@
> +        });
> +      } else if (msg.forget) {
> +        browser.sessions.forgetClosedWindow(msg.forget).then(() => {
> +          browser.test.sendMessage("forgot", msg.forget);
> +        }).catch(err => {

I don't think `catch` will work as expected here. Instead define a function that gets called on rejection. See http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_sessions_restore.js#29-38 for an example of this.

@@ +47,5 @@
> +      permissions: ["sessions", "tabs"],
> +    },
> +    background,
> +  });
> +

I think the body of the test should be changed significantly to simplify it and just test what needs to be tested. I suggest it should do something along these lines:

- Open and close a window with a single tab, twice.
- Call getRecentlyClosed and store the sessionIds for the two windows that were just closed, as well as the length of the array returned.
- Call forgetClosedWindow using the sessionId of the most recently closed window.
- Call getRecentlyClosed and verify that the most recently closed window (in the data returned) is the one we expect (i.e., the first closed window). Also check the complete data from getRecentlyClosed to ensure that there are no windows with the forgotten sessionId, and check that the length of the array is one less than the value we stored.
- Call forgetClosedWindow using the sessionId of the window that was forgotten.
- Verify that we get the expected rejection.

Comment 10

[Thomas Wisniewski]

Attachment: 1334266-add_WebExtension_support_for_forgetClosedTab_and_Window.diff

Alright, I had some time to address your review comments, :bsilverberg. Mind checking this version over to see if I missed anything?

Comment 11

[Kain]

Much excite on my end! Hoping to see this merged in soon!

Comment 12

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8854574 [details] [diff] [review]
1334266-add_WebExtension_support_for_forgetClosedTab_and_Window.diff

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

Nice work, Thomas. You've addressed my comments well, and I just have a few more comments about the tests.

In future, could you please use MozReview for submitting patches for review rather than just attaching diffs? It makes the review process a lot easier, especially when multiple iterations of reviews are necessary.

::: browser/components/extensions/test/browser/browser_ext_sessions_forgetClosedTab.js
@@ +1,1 @@
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */

I looked at browser_ext_sessions_forgetClosedWindow.js first, and all of my comments there apply to this file as well. Please take a look at those and change this file accordingly.

@@ +1,5 @@
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set sts=2 sw=2 et tw=80: */
> +"use strict";
> +
> +add_task(function* test_sessions_forget_closed_window() {

Change the test name to say `tab` instead of `window`.

::: browser/components/extensions/test/browser/browser_ext_sessions_forgetClosedWindow.js
@@ +21,5 @@
> +          () => {
> +            browser.test.sendMessage("forget-result", true);
> +          },
> +          error => {
> +            browser.test.sendMessage("forget-result", false);

Can you add a test that the error message that we receive is the one we expect from the rejection?

@@ +37,5 @@
> +  });
> +
> +  yield openAndCloseWindow("about:config");
> +  yield extension.startup();
> +  yield openAndCloseWindow("about:robots");

Why open and close one window before startup and one after?

@@ +43,5 @@
> +  extension.sendMessage("check-sessions");
> +  let recentlyClosed = yield extension.awaitMessage("recentlyClosed"),
> +      recentlyClosedLength = recentlyClosed.length,
> +      mostRecentlyClosedWindowSessionId =
> +        recentlyClosed.filter(r => !!r.window).pop().window.sessionId;

I'm not sure why you're going through all this trouble when we know that the most recently closed window is the one we care about. Why not just grab the sessionId from the first item in the recentlyClosed array?

@@ +51,5 @@
> +  is(yield extension.awaitMessage("forget-result"), true, "Forgot window");
> +  extension.sendMessage("check-sessions");
> +  let remainingClosed = yield extension.awaitMessage("recentlyClosed");
> +  is(remainingClosed.length, recentlyClosedLength-1, "One window was forgotten");
> +  is(remainingClosed.filter(r => r.window && r.window.sessionId ===

As above, this seems overly complicated. Why not just check the the sessionId of the first item in the array is the one we expect (i.e., the second item from the original array)?

Comment 13

[Thomas Wisniewski]

Attachment: 1334266-add_WebExtension_support_for_forgetClosedTab_and_Window.diff

>In future, could you please use MozReview for submitting patches for review rather than just attaching diffs?

Sure, I definitely need to learn how to use MozReview at some point. Thanks for putting up with attached diffs this time!

>I looked at browser_ext_sessions_forgetClosedWindow.js first, and all of my comments there apply to this file as well. Please take a look at those and change this file accordingly.

Both files are basically the same, aside from the obvious changes (open/closing tabs instead of windows, removing the now-unneeded openAndCloseWindow, sending two parameters to the forget method instead of one, and otherwise changing "window" to "tab".

>Can you add a test that the error message that we receive is the one we expect from the rejection?

Done.

>Why open and close one window before startup and one after?

No particular reason; I just felt it might be an extra small sanity check to ensure that things worked as expected that way. I've moved the open/closes to after the extension startup in this version, though.

>I'm not sure why you're going through all this trouble
>As above, this seems overly complicated.

Heh. I've simplified those in this version. I only went through that trouble because it seemed to be what you were suggesting to do in your first set of review comments :)

Let me know if anything else seems necessary; I'm not sure what kind of try-run you guys like to do for such WE patches, for instance (the new tests are passing locally for me, at least).

Comment 14

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8855117 [details] [diff] [review]
1334266-add_WebExtension_support_for_forgetClosedTab_and_Window.diff

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

This is getting very close, thanks for addressing all of my issues. As I mention below, there are a bunch of eslint issues that need to be fixed. Also, please apply the suggestions I have made for browser_ext_sessions_forgetClosedTab.js to browser_ext_sessions_forgetClosedWindow.js as well.

::: browser/components/extensions/test/browser/browser_ext_sessions_forgetClosedTab.js
@@ +14,5 @@
> +          () => {
> +            browser.test.sendMessage("forget-result");
> +          },
> +          error => {
> +            browser.test.sendMessage("forget-result", error.message);

Rather than sending the error message back to the test and checking it there, you can just check the error message here, before sending the forget-result message, via `browser.test.assertEq`.

@@ +31,5 @@
> +
> +  yield extension.startup();
> +
> +  let tabUrl = "http://example.com",
> +      tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, tabUrl);

Sorry for not pointing this out before, but our style dictates that we have only one assignment per `let`, so this, and a few other places where you've assigned multiple variables with a single `let`, need to be fixed.

@@ +44,5 @@
> +
> +  // Check that forgetting a tab works properly
> +  extension.sendMessage("forget-tab", recentlyClosedTab.windowId,
> +                                      recentlyClosedTab.sessionId);
> +  is(yield extension.awaitMessage("forget-result"), undefined, "Forgot tab");

This is a bit ugly. Rather than this you could have two different messages; `forgot-tab` could be sent when the call to `browser.sessions.forgetClosedTab` is successful and `forgot-reject` could be sent when the promise is rejected. Then you can simply await each message when it is expected and you don't need to worry about values at all.

@@ +47,5 @@
> +                                      recentlyClosedTab.sessionId);
> +  is(yield extension.awaitMessage("forget-result"), undefined, "Forgot tab");
> +  extension.sendMessage("check-sessions");
> +  let remainingClosed = yield extension.awaitMessage("recentlyClosed");
> +  is(remainingClosed.length, recentlyClosedLength-1, "One tab was forgotten");

You need spaces around the `-` here. This is an error that would be caught via an eslint check, and there are actually a number of eslint errors in this patch. Please check your code (which can be done via mach eslint) for eslint errors before submitting a patch.

@@ +48,5 @@
> +  is(yield extension.awaitMessage("forget-result"), undefined, "Forgot tab");
> +  extension.sendMessage("check-sessions");
> +  let remainingClosed = yield extension.awaitMessage("recentlyClosed");
> +  is(remainingClosed.length, recentlyClosedLength-1, "One tab was forgotten");
> +  is(remainingClosed[0].tab.sessionId, 0, "The correct tab was forgotten");

You cannot safely assume that the sessionId of this tab will be 0. You need to check the sessionId from the `recentlyClosed` that you saved earlier.

Comment 15

[Thomas Wisniewski]

Attachment: 1334266-add_WebExtension_support_for_forgetClosedTab_and_Window.diff

Alright, here's a fresh patch with the requested changes. Thanks for the patient reviewing!

Comment 16

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8856072 [details] [diff] [review]
1334266-add_WebExtension_support_for_forgetClosedTab_and_Window.diff

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

Looks good Thomas. Just a couple more minor things with the tests, but I do believe we are very close.

::: browser/components/extensions/test/browser/browser_ext_sessions_forgetClosedTab.js
@@ +46,5 @@
> +
> +  // Check that forgetting a tab works properly
> +  extension.sendMessage("forget-tab", recentlyClosedTab.windowId,
> +                                      recentlyClosedTab.sessionId);
> +  is(yield extension.awaitMessage("forgot-tab"), undefined, "Forgot tab");

We don't really need the `is` assertion here. You can just await the forgot-tab message. If the message doesn't arrive the test will fail with a timeout, so the "Forgot tab" assertion message will not appear to help debug the failure anyway.

@@ +50,5 @@
> +  is(yield extension.awaitMessage("forgot-tab"), undefined, "Forgot tab");
> +  extension.sendMessage("check-sessions");
> +  let remainingClosed = yield extension.awaitMessage("recentlyClosed");
> +  is(remainingClosed.length, recentlyClosedLength - 1,
> +     "One tab was forgotten");

I generally add periods to the end of my assertion messages. That's just a preference for me, so feel free to either change it or ignore my suggestion.

@@ +54,5 @@
> +     "One tab was forgotten");
> +  is(remainingClosed[0].tab.sessionId, recentlyClosed[1].tab.sessionId,
> +     "The correct tab was forgotten");
> +
> +  // Check that re-forgetting the same tab fails properly

As above, I try to add periods to the ends of my comments. This is just a suggestion you can implement or ignore.

@@ +63,5 @@
> +  remainingClosed = yield extension.awaitMessage("recentlyClosed");
> +  is(remainingClosed.length, recentlyClosedLength - 1,
> +     "No extra tab was forgotten");
> +  is(remainingClosed[0].tab.sessionId, recentlyClosed[1].tab.sessionId,
> +     "The correct tab remains.");

You should be consistent with periods though. Either use them everywhere or nowhere, please.

::: browser/components/extensions/test/browser/browser_ext_sessions_forgetClosedWindow.js
@@ +1,1 @@
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */

The same comments I made against browser_ext_sessions_forgetClosedTab.js apply here.

Comment 17

[Thomas Wisniewski]

Attachment: 1334266-add_WebExtension_support_for_forgetClosedTab_and_Window.diff

Alright, I went with consistent periods (and removed the pointless "is") in both .

Comment 18

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8857134 [details] [diff] [review]
1334266-add_WebExtension_support_for_forgetClosedTab_and_Window.diff

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

Looks good to me, Thomas. Thanks for keeping up with all of my comments. It still needs to be reviewed by a peer, so I'm going to flag Shane for that.

Comment 19

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8857134 [details] [diff] [review]
1334266-add_WebExtension_support_for_forgetClosedTab_and_Window.diff

LGTM.  push to try before landing please.

Comment 20

[Bob Silverberg [:bsilverberg]]

Thanks Shane. :)

Thomas, can you push your patch to try please?

Comment 21

[Thomas Wisniewski]

Sure. Is there a preferred command that I should push with?

Comment 22

[Bob Silverberg [:bsilverberg]]

If using mach try I tend to use:

./mach try -b do -p linux,linux64,macosx64,win32 -u xpcshell,mochitests -t none

That covers the major platforms and all of our test types.

Comment 23

[Thomas Wisniewski]

Pushed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eaaa5d765091fc3f1abe59e0ec17d7b9b69d43ad

Comment 24

[Kain]

This is awesome - assuming it gets merged in, in what version of FF would I expect to see this available?

Comment 25

[Bob Silverberg [:bsilverberg]]

The try run looks good. Requesting check in.

Great job Thomas! And thanks Kain for getting the ball rolling with this bug.

Comment 26

[Caitlin Neiman [:caitmuenster]]

\o/ Congrats on getting this uplifted!  

Kain and Thomas, I would love to help set you up on mozillians.org. If you want to create a profile and ping me, I would be happy to vouch for your awesome contributions.

Comment 27

[Thomas Wisniewski]

Oh, I'm already a Mozillian: I work on the web-compat platform team :)

(But I just activated my profile on mozillians.org, thanks for the reminder!)

Comment 28

[Caitlin Neiman [:caitmuenster]]

Oh awesome! My bad, Thomas. :) Thanks for activating your profile and jumping in on this bug!

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0602301ef1b2
Add support for WebExtension APIs browser.sessions.{forgetClosedTab|forgetClosedWindow}. r=bsilverberg, r=mixedpuppy

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0602301ef1b2

Comment 31

[Bob Silverberg [:bsilverberg]]

(In reply to Kain from comment #24)
> This is awesome - assuming it gets merged in, in what version of FF would I
> expect to see this available?

This will be in Firefox 55.

Comment 32

[Will Bamberg [:wbamberg]]

I've written a couple of pages on this:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sessions/forgetClosedTab
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sessions/forgetClosedWindow

Note that the compat data hasn't updated yet. Let me know if this covers it.

Comment 33

[Bob Silverberg [:bsilverberg]]

Looks good, thanks Will, and sorry for the delay reviewing.