1359806 - sessions.restore returns an array instead of an object

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P1)

Description

[Bob Silverberg [:bsilverberg]]

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 1

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1359806 - Fix sessions.restore to return an object instead of an array,

sessions.restore should always have returned an object, but landed returning an array.

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

Comment 2

[Shane Caraveo (:mixedpuppy)]

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

Comment 3

[Shane Caraveo (:mixedpuppy)]

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?

Comment 4

[Bob Silverberg [:bsilverberg]]

(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.

Comment 5

[Jorge Villalobos [:jorgev] (he/him)]

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?

Comment 6

[Bob Silverberg [:bsilverberg]]

(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

[u462496]

Shane, is this ready for checkin?

Comment 8

[Bob Silverberg [:bsilverberg]]

(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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff2157da9673
Fix sessions.restore to return an object instead of an array, r=mixedpuppy

Comment 10

[u462496]

(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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 12

[Bob Silverberg [:bsilverberg]]

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

Comment 13

[Bob Silverberg [:bsilverberg]]

Attachment: 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

Comment 14

[Gerry Chang [:gchang]]

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.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8f0099c322d3

Comment 16

[Bob Silverberg [:bsilverberg]]

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?

Comment 17

[Andrei Vaida [:avaida]]

(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.

Comment 18

[Jorge Villalobos [:jorgev] (he/him)]

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.