sessions.restore returns an array instead of an object

RESOLVED FIXED in Firefox 54

Status

()

Toolkit
WebExtensions: General
P1
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: bsilverberg, Assigned: bsilverberg)

Tracking

(Blocks: 1 bug, {addon-compat})

unspecified
mozilla55
addon-compat
Points:
---
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox54 fixed, firefox55 fixed)

Details

(Whiteboard: [sessions]triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

3 months ago
browser.sessions.restore should return an object as per [1] and [2], but it currently returns an array containing a single object. This should be fixed.

[1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sessions/restore
[2] http://searchfox.org/mozilla-central/source/browser/components/extensions/schemas/sessions.json#153
Comment hidden (mozreview-request)

Updated

3 months ago
Blocks: 1322060

Comment 2

3 months ago
mozreview-review
Comment on attachment 8861951 [details]
Bug 1359806 - Fix sessions.restore to return an object instead of an array,

https://reviewboard.mozilla.org/r/133958/#review136884
Attachment #8861951 - Flags: review?(mixedpuppy) → review+
That's a bummer.  Do you think this is used enough that some announcement should be made out to the mailing lists?  Or is it used so little someone just commented on the problem?
(Assignee)

Comment 4

3 months ago
(In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> That's a bummer.  Do you think this is used enough that some announcement
> should be made out to the mailing lists?  Or is it used so little someone
> just commented on the problem?

Yes, I am concerned about that too. Either nobody is using it, or people are ignoring the documentation and schema because it doesn't work as advertised.

I discussed it with Andy and he mentioned that we can search DXR to see if any extensions are currently using it. I guess we can either reach out to individual authors, or make a general announcement, based on those findings. He also suggested that Jorge might be the person to make that determination, so I am needinfoing him here as well. 

Oh, and thanks for the review. :)

Note that this API has been available since 52.
Flags: needinfo?(jorge)
We'll need to look into DXR (when it's working) to assess the impact and decide what to do about it.

What about Chrome compatibility? Is this bug going to make the API compatible with Chrome?
Flags: needinfo?(jorge)
Keywords: addon-compat
(Assignee)

Comment 6

3 months ago
(In reply to Jorge Villalobos [:jorgev] from comment #5)
> We'll need to look into DXR (when it's working) to assess the impact and
> decide what to do about it.
> 
> What about Chrome compatibility? Is this bug going to make the API
> compatible with Chrome?

Yes

Comment 7

3 months ago
Shane, is this ready for checkin?
Flags: needinfo?(mixedpuppy)
(Assignee)

Comment 8

3 months ago
(In reply to Kevin Jones from comment #7)
> Shane, is this ready for checkin?

I've already landed it, Kevin. It's on autoland now.
Flags: needinfo?(mixedpuppy)

Comment 9

3 months ago
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ff2157da9673
Fix sessions.restore to return an object instead of an array, r=mixedpuppy

Comment 10

3 months ago
(In reply to Bob Silverberg [:bsilverberg] from comment #8)
> (In reply to Kevin Jones from comment #7)
> > Shane, is this ready for checkin?
> 
> I've already landed it, Kevin. It's on autoland now.

Thanks Bob.  Sorry I was addressing the wrong person :-/

Comment 11

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ff2157da9673
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 12

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3278adb46811d4cb9a1aa4653177fe9a4b5d054
(Assignee)

Comment 13

3 months ago
Created attachment 8862624 [details] [diff] [review]
Uplift to beta

Approval Request Comment
[Feature/Bug causing the regression]: 1308060
[User impact if declined]: Users of the sessions.restore API will receive data in an unexpected format which is incorrect.
[Is this code covered by automated tests?]: Yes, a test exists at browser/components/extensions/test/browser/browser_ext_sessions_restore.js and was updated to cover this change. A try run has been started at https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed0d0f219d94b2e07dec9fda4f2542acce1a0a62
[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 two-line change to a single API method and it is covered by tests.
[String changes made/needed]: None
Attachment #8862624 - Flags: approval-mozilla-beta?

Updated

3 months ago
status-firefox54: --- → affected
Comment on attachment 8862624 [details] [diff] [review]
Uplift to beta

Fix a session.restore issue and include test. Beta54+. Should be in 54 beta 4.
Attachment #8862624 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 15

3 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/8f0099c322d3
status-firefox54: affected → fixed
Flags: in-testsuite+
(Assignee)

Comment 16

3 months ago
Jorge, this will land in 54. Can I assume that your adding the addon-compat keyword is enough to keep this on your radar for communication and possible outreach?
Flags: needinfo?(jorge)
(In reply to Bob Silverberg [:bsilverberg] from comment #13)
> [Is this code covered by automated tests?]: Yes, a test exists at
> browser/components/extensions/test/browser/browser_ext_sessions_restore.js
> and was updated to cover this change. A try run has been started at
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ed0d0f219d94b2e07dec9fda4f2542acce1a0a62
> [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-
The compatibility blog post for 54 was published a while ago, as the WebExtensions post for 54. Maybe this warrants an independent post. Anyway, it's on my radar.
Flags: needinfo?(jorge)
You need to log in before you can comment on or make changes to this bug.