1375546 - getRecentlyClosed({}) does not always list name and url from time when tab was closed

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P3)

Description

[Jan Zavřel]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170608105825

Steps to reproduce:

I open a new tab with a certain url. Then left-click hyperlinked text to open new url in the same tab. And again. So there is a tab with three urls in history. Then I close this tab.
Then I use sessions.getRecentlyClosed({}) to list all closed tabs in the grid of my addon (http://tinyurl.com/at-your-command).
Then I select a tab in the grid and use sessions.restore() to restore tab.


Actual results:

My experience up to this time: item.tab.url provided by getRecentlyClosed({}) is not always url from when tab was closed. The same goes for name.
While sessions.restore() quite reliable restore tab with url (history position) from time when tab was closed.


Expected results:

getRecentlyClosed({}) should always list name and url from time when tab was closed

Comment 1

[Jan Zavřel]

Please, contact me, should you need more information.

Comment 2

[Jan Zavřel]

Can always reproduce it like this:
1) open url
2) click hyperlinked text to show new url in same tab
3) click back
4) close tab
5) second tab name and url is provided by sessions.getRecentlyClosed({})
6) but restore API shows first url

expected:
first tab name and url is provided by sessions.getRecentlyClosed({}), i.e. name and url from time when tab was closed

Comment 3

[Jan Zavřel]

Hello,
can you please at least confirm this bug?
BR
JZ

Comment 4

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1375546 - Fix sessions.getRecentlyClosed to consider the current tab history index

If the back button is used to navigate a tab backwards in its history, and then the tab is closed,
the current implementation of sessions.getRecentlyClosed does not take this into account and
just uses the last item in the tab history for the values returned for the tab. This patch fixes
this by using the values for the current position of the tab in its history.

Review commit: https://reviewboard.mozilla.org/r/181770/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181770/

Comment 5

[Bob Silverberg [:bsilverberg]]

Mike, I believe I am looking at the correct data to determine which is the current entry from the tab's history, although I am puzzled by the fact that the index seems to be 1-based rather than 0-based. Requesting review from you to confirm that the logic that I've added to ext-browser.js to locate the correct history entry is accurate.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8910273 [details]
Bug 1375546 - Fix sessions.getRecentlyClosed to consider the current tab history index

https://reviewboard.mozilla.org/r/181770/#review187156

This looks good, however I'm not familiar with the tab session data, so maybe mikedeboer can still take a look at it from that perspective.

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 8910273 [details]
Bug 1375546 - Fix sessions.getRecentlyClosed to consider the current tab history index

https://reviewboard.mozilla.org/r/181770/#review187498

At the time when we implemented the API together, I was not aware enough of the sessionstore subtleties like this one. Otherwise I would've raised this and the fact that you'll want to add a test to iterate and check a fully populated sessionstore. Oh well, glad you fixed it today, though!

::: browser/components/extensions/ext-browser.js:668
(Diff revision 1)
>        lastAccessed: tabData.state ? tabData.state.lastAccessed : tabData.lastAccessed,
>      };
>  
>      if (extension.tabManager.hasTabPermission(tabData)) {
>        let entries = tabData.state ? tabData.state.entries : tabData.entries;
> -      let entry = entries[entries.length - 1];
> +      let lastTabIndex = tabData.state ? tabData.state.index : tabData.index;

This looks correct to me, but please add a comment as to _why_ you need to do `index - 1`.

Comment 8

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8910273 [details]
Bug 1375546 - Fix sessions.getRecentlyClosed to consider the current tab history index

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181770/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae7f9f3bbec2
Fix sessions.getRecentlyClosed to consider the current tab history index, r=mikedeboer,mixedpuppy

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ae7f9f3bbec2

Comment 11

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8910273 [details]
Bug 1375546 - Fix sessions.getRecentlyClosed to consider the current tab history index

Approval Request Comment
[Feature/Bug causing the regression]: 1338312
[User impact if declined]: the sessions.getRecentlyClosed API will return an incorrect URL for closed tabs that have been navigated via the back button.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No 
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It only affects the getRecentlyClosed method of the sessions API and it is covered via tests.
[String changes made/needed]: None

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8910273 [details]
Bug 1375546 - Fix sessions.getRecentlyClosed to consider the current tab history index

Fix a bug in the WebExtension support, taking it.
Should be in 57b3

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c270c61e9015

Comment 14

[Andrei Vaida [:avaida]]

(In reply to Bob Silverberg [:bsilverberg] from comment #11)
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]: No
> [Needs manual test from QE? If yes, steps to reproduce]: No 

Setting qe-verify- based on Bob's assessment on manual testing needs and the fact that this fix has automated coverage.