Closed Bug 1337509 Opened 6 years ago Closed 5 years ago

Do not create tab objects with an unsupported `selected` property

Categories

(WebExtensions :: General, defect, P5)

Product:
WebExtensions
Component:
General
Type:
defect

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: bsilverberg, Assigned: shatur, Mentored)

Details

(Keywords: good-first-bug, Whiteboard: triaged)

Attachments

(1 file)

Bug 1337509 - Do not create tab objects with an unsupported 'selected' property.
59 bytes, text/x-review-board-request
bsilverberg
: review+
mixedpuppy
: review+
Details 
When we create tab objects in API code, for example in convert [1] and convertFromSessionStoreClosedData [2], we create a tab object that has a `selected` property. In the tabs schema, `selected` is marked as unsupported [3].

This came to light when someone was working on an experiment that wanted to accept a tab object as a function parameter. The tab object was failing schema validation because it had an unsupported `selected` property. This issue was resolved by using the tabId rather than a tab object, which is what we do in all of our other existing API code, but it still seems like an issue.

Also, if we were ever to add code to validate objects being returned by an API call against the schema, all of our tab objects would fail.

Is this something we want to fix, and if so, what is the fix? Should we just remove unsupported from the `selected` property in the schema?
 
[1] http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ExtensionTabs.jsm#110
[2] http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-utils.js#451
[3] http://searchfox.org/mozilla-central/source/browser/components/extensions/schemas/tabs.json#63
(In reply to Bob Silverberg [:bsilverberg] from comment #0)
> what is the fix? Should we just remove unsupported from the `selected`
> property in the schema?

No, we should just remove the "selected" property from the tab objects we return. It's deprecated in favor of "active".
Thanks Kris. That was another option I considered, but I assumed that the reason we were putting it there in the first place was for compatibility. `selected` is also deprecated in Chrome, but only deprecated, so an extension could very well expect it to exist. Are we concerned about just removing it entirely from the tab objects?
(In reply to Bob Silverberg [:bsilverberg] from comment #2)
> Are we concerned about just removing it entirely from the tab objects?

No. Our policy has been not to implement deprecated parts of the Chrome APIs.
Thanks again Kris. I am updating this bug to be about not creating tab objects with a `selected` property, and I've marked it as a good-first-bug. Here are some implementation details:

We'll need to change the code that creates tab objects to omit the `selected` property. As of this writing, that code exists at:

http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ExtensionTabs.jsm#115
http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-utils.js#456

Tests will also need to be updated to check that tab objects are not being created with a `selected` property.
Mentor: bob.silverberg
Keywords: good-first-bug
Priority: -- → P5
Summary: Tab objects are being created with an unsupported `selected` property → Do not create tab objects with an unsupported `selected` property
Whiteboard: triaged
can i work on this bug?
Please do. Let us know if you have any questions.

Thanks!
Assignee: nobody → amanaryan23
Status: NEW → ASSIGNED
do i have to remove(or comment out) all the functions that use the 'selected' property in both the files? please correct me if i am wrong.
(In reply to Aman Kumar from comment #7)
> do i have to remove(or comment out) all the functions that use the
> 'selected' property in both the files? please correct me if i am wrong.

Hi Aman, my apologies for not replying sooner. You will want to change any functions that create tab properties to *not* include a selected property when creating the tab. I believe I have identified those about in comment #4. You shouldn't have to change any other places the reference the selected property.

Also, as mentioned in comment #4, some tests will need to be updated to verify this change, but you could try just making the change first and we can talk about the tests later.
Flags: needinfo?(amanaryan23)
(In reply to Bob Silverberg [:bsilverberg] from comment #8)
> (In reply to Aman Kumar from comment #7)
> > do i have to remove(or comment out) all the functions that use the
> > 'selected' property in both the files? please correct me if i am wrong.
> 
> Hi Aman, my apologies for not replying sooner. You will want to change any
> functions that create tab properties to *not* include a selected property
> when creating the tab. I believe I have identified those about in comment
> #4. You shouldn't have to change any other places the reference the selected
> property.
> 
> Also, as mentioned in comment #4, some tests will need to be updated to
> verify this change, but you could try just making the change first and we
> can talk about the tests later.

I am having confusion in ext-util.js where there are two functions 'get selected()' and 'get active()'
should i remove both because both use selected property?
(In reply to Aman Kumar from comment #9)
> 
> I am having confusion in ext-util.js where there are two functions 'get
> selected()' and 'get active()'
> should i remove both because both use selected property?

No, you don't actually need to change any of that. It's only the places where we are creating a tab object and returning it where we want to leave out the `selected` property.

Specifically, in the `convertFromSessionStoreClosedData` function in `ext-utils.js` here: http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-utils.js#538

and in the `convert` function in `ExtensionTabs.jsm` here: http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ExtensionTabs.jsm#463

If you simply remove those lines in those functions, we will no longer be generating tab objects with a selected property.
Hi Bob!

About tests, do I have to modify all tests, which uses the tab object or any one will do?

Regards.
Hi Tushar,

Thanks for taking the time to work on this bug. This bug was assigned to Aman Kumar who I believe started working on it, and we haven't heard anything back from them in over a month, but I think it would be respectful to give then a chance to pick it up again before handing it off to you.

Aman, if you would like to continue working on this please let me know within the next couple of days. If I do not hear back from you I will reassign it to Tushar.
(In reply to Bob Silverberg [:bsilverberg] from comment #13)
Hey!
> Thanks for taking the time to work on this bug. This bug was assigned to
> Aman Kumar who I believe started working on it, and we haven't heard
> anything back from them in over a month, but I think it would be respectful
> to give then a chance to pick it up again before handing it off to you.

Sorry for that, from next time I will be more careful and comment before picking up a bug Aman please continue, if you are up for it.:-)

Thanks!
No worries, Tushar. I assume that Aman will not be finishing this up, but I wanted to be sure first.
Tushar, I have re-assigned the bug to you. Thanks for your efforts thus far. The changes to the code look good. As for tests, I don't think we need to change all tests that use tabs, but we should make sure that at least one test which uses tabs produced from each code path is either updated or added to assert that the `selected` property does not exist.
Assignee: amanaryan23 → tushar.saini1285
Flags: needinfo?(amanaryan23) → needinfo?(tushar.saini1285)
Comment on attachment 8853634 [details]
Bug 1337509 - Do not create tab objects with an unsupported 'selected' property.

Clearing my review flag for now, until the next version, with tests, is ready for review.
Attachment #8853634 - Flags: review?(bob.silverberg)
Hi!

Included the tests to check 'selected' property does not exist. Please let me know if further changes are required.

Thanks!
Flags: needinfo?(tushar.saini1285)
Comment on attachment 8853634 [details]
Bug 1337509 - Do not create tab objects with an unsupported 'selected' property.

https://reviewboard.mozilla.org/r/125716/#review130960

This looks good, Tushar. Just a couple of minor comments on the tests to address. Thanks for your work on this!

::: browser/components/extensions/test/browser/browser_ext_sessions_getRecentlyClosed_tabs.js:60
(Diff revision 2)
>    let recentlyClosed = await extension.awaitMessage("recentlyClosed");
>    let tabInfo = recentlyClosed[0].tab;
>    let expectedTab = expectedTabs.pop();
>    checkTabInfo(expectedTab, tabInfo);
>  
> +  // Verify tab objects are not being created with a `selected` property.

You could accomplish this, and also the check you added at lines 73-74, by simply updating the created expectedTabInfo for each tab to include `selected: undefined`. You could add that at line 11 in the test.

::: browser/components/extensions/test/browser/browser_ext_tabs_create.js:128
(Diff revision 2)
>                browser.tabs.create(test.create),
>                createdPromise,
>              ]);
>              let tabId = tab.id;
>  
> +            // Verify tab objects are not being created with a `selected` property.

Similar to my comment on browser_ext_sessions_getRecentlyClosed_tabs.js, you could test for this by simply adding `selected: false` to the `DEFAULTS` object on line 38.
Attachment #8853634 - Flags: review?(bob.silverberg) → review-
Comment on attachment 8853634 [details]
Bug 1337509 - Do not create tab objects with an unsupported 'selected' property.

https://reviewboard.mozilla.org/r/125716/#review131004

This looks good Tushar, nice work! I'm going to request a try run and also ask one of my colleagues for a final r+.
Attachment #8853634 - Flags: review?(bob.silverberg) → review+
Comment on attachment 8853634 [details]
Bug 1337509 - Do not create tab objects with an unsupported 'selected' property.

Shane, could you please give this a r? to see if I've missed anything?
Attachment #8853634 - Flags: review?(mixedpuppy)
Comment on attachment 8853634 [details]
Bug 1337509 - Do not create tab objects with an unsupported 'selected' property.

Actually, there's something wrong with the test. It passed for me locally, but is failing on try, so I'll take another look before requesting a final review.
Attachment #8853634 - Flags: review?(mixedpuppy)
Ah, I see. It's not a problem with the test that was updated. It's a problem with the tests that were not updated. Some other tests need to be changed as well. It looks like the following tests also need to be updated:

browser_ext_sessions_getRecentlyClosed.js
browser_ext_sessions_getRecentlyClosed_private.js
browser_ext_tabs_duplicate.js

Can you please update those as well, Tushar?
Flags: needinfo?(tushar.saini1285)
Done necessary changes! :)
Flags: needinfo?(tushar.saini1285)
Comment on attachment 8853634 [details]
Bug 1337509 - Do not create tab objects with an unsupported 'selected' property.

https://reviewboard.mozilla.org/r/125716/#review132006

Looks good, Tushar. Just a few changes needed to some of the tests and we should be good to go.

::: browser/components/extensions/test/browser/browser_ext_tabs_create.js:44
(Diff revision 4)
>              index: 2,
>              windowId: activeWindow,
>              active: true,
>              pinned: false,
>              url: "about:newtab",
> +            selected: undefined,

This change is unnecesary, unless this test is failing for you without the change.

::: browser/components/extensions/test/browser/browser_ext_tabs_duplicate.js:25
(Diff revision 4)
>          browser.tabs.move(source.id, {index: 0}, () => {
>            browser.tabs.duplicate(source.id, (tab) => {
>              browser.test.assertEq("http://example.net/", tab.url);
>              // Should be the second tab, next to the one duplicated.
>              browser.test.assertEq(1, tab.index);
> -            // Should be selected by default.
> +            browser.test.assertEq(undefined, tab.selected);

Just remove this assertion entirely. We have another test that verifies that created tabs don't have a `selected` property.

::: browser/components/extensions/test/browser/head_sessions.js:30
(Diff revision 4)
>  
>  function checkTab(tab, windowId, incognito) {
> -  for (let prop of ["selected", "highlighted", "active", "pinned"]) {
> +  for (let prop of ["highlighted", "active", "pinned"]) {
>      is(tab[prop], false, `closed tab has the expected value for ${prop}`);
>    }
> +  is(tab.selected , undefined, "closed tab has the expected value for selected");

I think just removing `selected` from the list of props above is sufficient. We don't need to explicitly check that `selected` is undefined here.
Attachment #8853634 - Flags: review+ → review-
Comment on attachment 8853634 [details]
Bug 1337509 - Do not create tab objects with an unsupported 'selected' property.

https://reviewboard.mozilla.org/r/125716/#review132006

> This change is unnecesary, unless this test is failing for you without the change.

I think this is required, as this is the only place where we are testing for tabs object has no 'selected' property. Please, correct me if I am wrong.
Comment on attachment 8853634 [details]
Bug 1337509 - Do not create tab objects with an unsupported 'selected' property.

https://reviewboard.mozilla.org/r/125716/#review132006

> I think this is required, as this is the only place where we are testing for tabs object has no 'selected' property. Please, correct me if I am wrong.

Sorry, my mistake. I didn't realize that this was the first test you changes to test this.
Comment on attachment 8853634 [details]
Bug 1337509 - Do not create tab objects with an unsupported 'selected' property.

https://reviewboard.mozilla.org/r/125716/#review132504

I think it's good now. I will submit another try run, and if it's clean will ask for a final review. Thanks Tushar!
Attachment #8853634 - Flags: review?(bob.silverberg) → review+
Comment on attachment 8853634 [details]
Bug 1337509 - Do not create tab objects with an unsupported 'selected' property.

It looks good on try. Flagging Shane for a final review.
Attachment #8853634 - Flags: review?(mixedpuppy)
Comment on attachment 8853634 [details]
Bug 1337509 - Do not create tab objects with an unsupported 'selected' property.

https://reviewboard.mozilla.org/r/125716/#review133474

r+ with comments addressed.

::: browser/components/extensions/test/browser/browser_ext_tabs_create.js:44
(Diff revision 5)
>              index: 2,
>              windowId: activeWindow,
>              active: true,
>              pinned: false,
>              url: "about:newtab",
> +            selected: undefined,

I'm not convinced we need this in the tests, but I would at least like to see a comment so it isn't confusing at some point in the future.

::: browser/components/extensions/test/browser/browser_ext_tabs_duplicate.js
(Diff revision 5)
>            browser.tabs.duplicate(source.id, (tab) => {
>              browser.test.assertEq("http://example.net/", tab.url);
>              // Should be the second tab, next to the one duplicated.
>              browser.test.assertEq(1, tab.index);
> -            // Should be selected by default.
> -            browser.test.assertTrue(tab.selected);

Unless the behavior is has changed, change the to correct usage (tab.active)
Attachment #8853634 - Flags: review?(mixedpuppy) → review+
Hey, I have modified the patch as per the suggestions of Shane. Please have a look and let me know if further modification is required. Thanks!
Flags: needinfo?(bob.silverberg)
Comment on attachment 8853634 [details]
Bug 1337509 - Do not create tab objects with an unsupported 'selected' property.

https://reviewboard.mozilla.org/r/125716/#review134406
Attachment #8853634 - Flags: review+
Shane gave it a once over and an r+, so it's good to land. I'll do that. Thanks for all your work on this, Tushar.
Flags: needinfo?(bob.silverberg)
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e9a5d4f62461
Do not create tab objects with an unsupported 'selected' property. r=bsilverberg,mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/e9a5d4f62461
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1372110
No longer depends on: 1372110
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.