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

RESOLVED FIXED in Firefox 55

Status

P3
normal
RESOLVED FIXED
2 years ago
9 months ago

People

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

Tracking

53 Branch
mozilla55

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

Comment 3

2 years ago
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

Updated

2 years ago
Assignee: nobody → mark
Component: WebExtensions: Untriaged → WebExtensions: Developer Tools
Priority: -- → P3
Whiteboard: triaged
(Assignee)

Updated

2 years ago
Assignee: mark → amckay
(Assignee)

Comment 4

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

Comment 5

2 years ago
Comment hidden (mozreview-request)

Comment 7

2 years ago
mozreview-review
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

2 years ago
mozreview-review
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 hidden (mozreview-request)

Comment 10

2 years ago
mozreview-review
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

2 years ago
mozreview-review
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+
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 13

2 years ago
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

Comment 14

2 years ago
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
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Comment 16

2 years ago
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.
status-firefox53: affected → wontfix
status-firefox54: --- → wontfix
(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)
(Assignee)

Updated

2 years ago
Flags: needinfo?(amckay)

Updated

9 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.