Closed Bug 1338312 Opened 3 years ago Closed 3 years ago

browser.sessions.getRecentlyClosed returns incorrect information for tabs in closed windows

Categories

(WebExtensions :: General, defect, P1)

54 Branch
x86_64
Windows 10
defect

Tracking

(firefox53 fixed, firefox54 fixed)

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: kain_savage, Assigned: bsilverberg)

Details

(Whiteboard: [sessions]triaged)

Attachments

(2 files)

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

Steps to reproduce:

1. Create a web extension which adds a listener for `browser.windows.onRemoved` which calls `browser.sessions.getRecentlyClosed` and logs the first entry
2. Open a new window
3. Visit www.google.com
4. Close the window


Actual results:

The log shows that there is a single tab in the `tabs` array on the window object, and that tab has a `title` of "Nightly Start Page" and a `url` of "about:home".


Expected results:

The log should show a single tab in the `tabs` array on the window object, and that tab should have a `title` of "Google" and a `url` of "https://www.google.com/"
The actual result is the same when using `browser.sessions.onChanged` instead of `browser.windows.onRemoved` - they should both have the same expected results as above.
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64
Version: 53 Branch → 54 Branch
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit
Assignee: nobody → bob.silverberg
Whiteboard: investigation
As of today, this bug is also affecting `browser.tabs.onRemoved` (not previously affected at all).

Another interesting aspect is that `favIconUrl` is correct even though `url` and `title` are not.
(In reply to Kain from comment #2)
> As of today, this bug is also affecting `browser.tabs.onRemoved` (not
> previously affected at all).

I guess ignore this - today's nightly build seems to have resolved the `tab` issue.

`windows.onRemoved` is still bugged.
Status: UNCONFIRMED → NEW
Component: WebExtensions: Untriaged → WebExtensions: General
Ever confirmed: true
Priority: -- → P1
Summary: browser.windows.onRemoved callback bug → browser.sessions.getRecentlyClosed returns incorrect information for tabs in closed windows
Whiteboard: investigation → [sessions]triaged
The actual issue is that when a tab has been navigated, and the windows containing that tab has been closed, `getRecentlyClosed` reports the tab's url and title as the first item in the tab's history instead of the last item. I am attaching a patch that fixes this.
Comment on attachment 8837299 [details]
Bug 1338312 - browser.sessions.getRecentlyClosed returns incorrect information for tabs in closed windows,

https://reviewboard.mozilla.org/r/112458/#review114716

::: browser/components/extensions/ext-utils.js:547
(Diff revision 1)
>        incognito: Boolean(tabData.state && tabData.state.isPrivate),
>      };
>  
>      if (extension.tabManager.hasTabPermission(tabData)) {
>        let entries = tabData.state ? tabData.state.entries : tabData.entries;
> -      result.url = entries[0].url;
> +      let entry = entries[entries.length - 1];

is it possible that entries is empty here?
Attachment #8837299 - Flags: review?(aswan) → review+
Comment on attachment 8837299 [details]
Bug 1338312 - browser.sessions.getRecentlyClosed returns incorrect information for tabs in closed windows,

https://reviewboard.mozilla.org/r/112458/#review114716

> is it possible that entries is empty here?

I checked with Mike deBoer and he confirmed that it is not possible for `entries` to be empty here because we are dealing with a closed object.
Try looks good, requesting check in.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e2b89b2e9dd7
browser.sessions.getRecentlyClosed returns incorrect information for tabs in closed windows, r=aswan
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e2b89b2e9dd7
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Attached patch Uplift to AuroraSplinter Review
Approval Request Comment
[Feature/Bug causing the regression]: 1338312
[User impact if declined]: This patch addresses a bug in the WebExtensions sessions API which cases an API call to return incorrect data for tabs in closed windows. An add-on developer using that call would receive incorrect data.
[Is this code covered by automated tests?]: Yes, new tests were added that cover all of the code changes.
[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?]: This is a small change to a single method in a file which implements some WebExtension helpers. The changes are fully covered by automated tests. A try run has been submitted at https://treeherder.mozilla.org/#/jobs?repo=try&revision=87edce43bbfa7f4720f840191a8d980d700d4384
[String changes made/needed]: None
Attachment #8839926 - Flags: approval-mozilla-aurora?
Hi Bob,
The [Feature/Bug causing the regression] should not be the bug itself. Can you let me know what is the feature or the bug cause this issue?
Flags: needinfo?(bob.silverberg)
(In reply to Gerry Chang [:gchang] from comment #14)
> Hi Bob,
> The [Feature/Bug causing the regression] should not be the bug itself. Can
> you let me know what is the feature or the bug cause this issue?

Sorry, the bug number for the feature was 1308058.
Flags: needinfo?(bob.silverberg)
Comment on attachment 8839926 [details] [diff] [review]
Uplift to Aurora

Fix an issue in the WebExtensions sessions API. Aurora53+.
Attachment #8839926 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.