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

RESOLVED FIXED in Firefox 55

Status

defect
P3
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: vasilica.mihasca, Assigned: andy+bugzilla)

Tracking

53 Branch
mozilla55
Dependency tree / graph

Firefox Tracking Flags

(firefox50 unaffected, firefox51 unaffected, firefox52 unaffected, firefox53 wontfix, firefox54 wontfix, firefox55 fixed)

Details

(Whiteboard: triaged)

Attachments

(3 attachments)

[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
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.
Flags: needinfo?(eglassercamp) → needinfo?(amckay)
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.
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.
Flags: needinfo?(amckay)
Keywords: regression
Summary: No data fields displayed in add-on details tab for a webextension without id → Warn developer in about:debugging if using chrome.storage.sync with a temporary id
Assignee: nobody → mark
Component: WebExtensions: Untriaged → WebExtensions: Developer Tools
Priority: -- → P3
Whiteboard: triaged
Assignee: mark → amckay
Sorry, hope you weren't working on this mark, I forgot we'd assigned you this bug. Attached a screenshot and some code.
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 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.
Attachment #8867406 - Flags: review?(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 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.
Attachment #8867406 - Flags: review?(mstriemer) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de652bd2760d
Add a warning for temporary ids in about:debugging. r=mstriemer
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/26b098924b0a
Fix space-infix-ops ESLint error in browser_addons_debug_info.js.
https://hg.mozilla.org/mozilla-central/rev/de652bd2760d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Pushed by archaeopteryx@coole-files.de:
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
Too late for uplift to 54 so it looks like this fix will ride to release with 55.
(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 :)
Flags: needinfo?(mstriemer)
Flags: needinfo?(amckay)
Flags: needinfo?(mstriemer)
Flags: needinfo?(amckay)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.