Closed Bug 1375546 Opened 2 years ago Closed 2 years ago

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

Categories

(WebExtensions :: General, defect, P3)

defect

Tracking

(firefox57 fixed, firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: jzav, Assigned: bsilverberg)

References

(Blocks 1 open bug)

Details

(Whiteboard: investigate)

Attachments

(1 file)

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
Please, contact me, should you need more information.
Assignee: nobody → bob.silverberg
Component: Untriaged → WebExtensions: General
Product: Firefox → Toolkit
Whiteboard: investigate
Version: 54 Branch → unspecified
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
Hello,
can you please at least confirm this bug?
BR
JZ
Priority: -- → P3
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.
Flags: needinfo?(mdeboer)
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.
Attachment #8910273 - Flags: review?(mixedpuppy) → review+
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`.
Attachment #8910273 - Flags: review?(mdeboer) → review+
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(mdeboer)
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ae7f9f3bbec2
Fix sessions.getRecentlyClosed to consider the current tab history index, r=mikedeboer,mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/ae7f9f3bbec2
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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
Attachment #8910273 - Flags: approval-mozilla-beta?
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
Attachment #8910273 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: qe-verify-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.