1326230 - Warn developer in about:debugging if using chrome.storage.sync with a temporary id

Status: RESOLVED FIXED

(WebExtensions :: Developer Tools, defect, P3)

Description

[Vasilica Tamas, QA [:vtamas]]

Attachment: Category-Tabs-for-Google-Keep™_v3.1.0.xpi

[Prerequisites]:
- webextensions.storage.sync.enabled set to true
- create a new string identity.fxaccounts.autoconfig.uri and set it to https://stable.dev.lcip.org

[Affected versions]:
Firefox 53.0a1 (2016-11-29)

[Affected platforms]:
Windows 10 64-bit
Ubuntu 16.04 32-bit

[Steps to reproduce]:
1.Launch Firefox with a clean profile and set up the configuration.
2.Go to about:preferences#sync and login using a valid FxA account. 
3.Install the attached webextension.
4.Navigate to the add-on details tab from about:addons 

[Expected Results]:
Category setup fields are successfully displayed in add-on details tab from about:addons: http://screencast.com/t/BGteZ7bYrC

[Actual Results]:
- There are no category setup fields in add-on details tab from Add-ons Manager.
- See screenshot: http://screencast.com/t/G84U7xjOgU
- The following error message appears in Browser Console: n is undefined  background.js:1


[Regression Range]:
Last good revision: 5e444885af53c55d7835b42846fac1fd7c990dbf
First bad revision: 0dab09cc7ea557361c8baa6ea159e0c8493859db
Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5e444885af53c55d7835b42846fac1fd7c990dbf&tochange=0dab09cc7ea557361c8baa6ea159e0c8493859db

Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1323228

Comment 1

[Ethan Glasser-Camp (:glasserc)]

I believe this is working as designed now. If an addon developer would like to test their addon and that addon uses chrome.storage.sync, they need to edit their addon's manifest to provide an ID.

Comment 2

[Kris Maglione [:kmag]]

I think we should consider allowing use of the storage.sync API, but not actually syncing the data to a remote server, in these cases. A console warning would probably be appropriate, too.

Comment 3

[Andy McKay]

Either way this is a matter of informing the developer whats going on so that its clear to them. We've already added a warning on this to the linter. I think we should go for improving about:debugging so that it clearly shows that the add-on is temporarily loaded and the consequences of that.

Comment 4

[Andy McKay]

Sorry, hope you weren't working on this mark, I forgot we'd assigned you this bug. Attached a screenshot and some code.

Comment 5

[Andy McKay]

Attachment: Screenshot 2017-05-12 15.56.51.png

Comment 6

[Andy McKay]

Attachment: bug 1326230 add a warning for temporary ids in about:debugging

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

Comment 7

[Mark Striemer [:mstriemer]]

Comment on attachment 8867406 [details]
bug 1326230 add a warning for temporary ids in about:debugging

https://reviewboard.mozilla.org/r/138948/#review143216

I think it makes sense to do the check in the react file rather than have to pass that data all the way through.

::: devtools/client/aboutdebugging/aboutdebugging.css:214
(Diff revision 1)
>    align-items: center;
> +  margin-top: 1em;
> +  margin-bottom: 1em;
> +  background-image: url(chrome://devtools/skin/images/help.svg);
> +  background-repeat: no-repeat;
> +  padding-left: 32px;

This should be `padding-inline-start` instead.

::: devtools/client/aboutdebugging/components/addons/target.js:74
(Diff revision 1)
>      ),
>    ];
>  }
>  
> +function temporaryID(target) {
> +  if (!target.temporaryID) {

I think you can import `AddonManagerPrivate` in this file and check `AddonManagerPrivate.isTemporaryInstallID(target.addonID)` here instead of adding the property in form.

::: devtools/client/aboutdebugging/components/addons/target.js:100
(Diff revision 1)
>      target: PropTypes.shape({
>        addonActor: PropTypes.string.isRequired,
>        addonID: PropTypes.string.isRequired,
>        icon: PropTypes.string,
>        name: PropTypes.string.isRequired,
> +      isTemporaryID: PropTypes.bool,

The property name is actually `temporaryID`. You could remove this if you do the check in this file though.

Comment 8

[Mark Striemer [:mstriemer]]

Comment on attachment 8867406 [details]
bug 1326230 add a warning for temporary ids in about:debugging

https://reviewboard.mozilla.org/r/138948/#review143598

::: devtools/client/aboutdebugging/components/addons/target.js:74
(Diff revision 1)
>      ),
>    ];
>  }
>  
> +function temporaryID(target) {
> +  if (!target.temporaryID) {

Actually the code that uses `AddonManager` is in devtools/client/aboutdebugging/modules/addon.js, the code to do the check can go in there and this can import it.

Comment 9

[Andy McKay]

Comment on attachment 8867406 [details]
bug 1326230 add a warning for temporary ids in about:debugging

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138948/diff/1-2/

Comment 10

[Mark Striemer [:mstriemer]]

Comment on attachment 8867406 [details]
bug 1326230 add a warning for temporary ids in about:debugging

https://reviewboard.mozilla.org/r/138948/#review149388

::: devtools/client/aboutdebugging/test/browser.ini:12
(Diff revision 2)
>    addons/unpacked/install.rdf
>    addons/bad/manifest.json
>    addons/bug1273184.xpi
>    addons/test-devtools-webextension/*
>    addons/test-devtools-webextension-nobg/*
> +  addons/test-devtools-webextension-temporary/*

I think just calling this "temporary" is a bit confusing since we test a lot of temporarily installed add-ons.

Maybe something like test-devtools-webextension-noid would be better?

Comment 11

[Mark Striemer [:mstriemer]]

Comment on attachment 8867406 [details]
bug 1326230 add a warning for temporary ids in about:debugging

https://reviewboard.mozilla.org/r/138948/#review149390

Just the nit on the test add-on's name. Looks good.

Comment 12

[Andy McKay]

Comment on attachment 8867406 [details]
bug 1326230 add a warning for temporary ids in about:debugging

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138948/diff/2-3/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de652bd2760d
Add a warning for temporary ids in about:debugging. r=mstriemer

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/26b098924b0a
Fix space-infix-ops ESLint error in browser_addons_debug_info.js.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/6d4b65abc362
Fix space-infix-ops ESLint error in browser_addons_debug_info.js. r=eslint-fix a=eslint-fix

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/26b098924b0a

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for uplift to 54 so it looks like this fix will ride to release with 55.

Comment 19

[Julian Descottes [:jdescottes]]

(came across code changes coming from this bug while reviewing Bug 1330732)

Can you ask for review from a DevTools peer when modifying about:debugging? (https://wiki.mozilla.org/Modules/All#DevTools)
For about:debugging you can usually ask me or :ochameau. Thanks!

ni? just to make sure you see the message :)