1419109 - empty default icon in manifest produces blank icon in toolbar

Status: VERIFIED FIXED

(WebExtensions :: Frontend, enhancement, P5)

Description

[Shane Caraveo (:mixedpuppy)]

having "default_icon": "" in the manifest produces buttons with no icon.  Happens with page/browser/sidebar actions.

We should probably disallow an empty value in the schema, but should also handle that in code.

Making this low priority since the extension can address this by not doing that.

Comment 1

[MarkBauer]

Alternatively, one could also provide a default icon (like a jigsaw piece, for starters), no?
I don't remember what Chrome does in that case, but would love to further investigate this and work on the implementation.

Comment 2

[clintontak22]

If this issue is still open I would like to look into fixing it.

Comment 3

[Caitlin Neiman [:caitmuenster]]

(In reply to clintontak22 from comment #2)
> If this issue is still open I would like to look into fixing it.

Hey there, sorry for the late reply! This issue is still open -- if you're still interested in working on it, please feel welcome to assign it to yourself! 

If you haven't already done so, you might also want to take a moment to review some tips for onboarding to the Firefox code base: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction

Comment 4

[Caitlin Neiman [:caitmuenster]]

Hey Shane! Can you provide some pointers for how a contributor could get started on this bug? 

If this is your first contribution, please see https://wiki.mozilla.org/WebExtensions/Contribution_Onramp for instructions on how to set up your environment.

Comment 5

[Varun [he/him IST]]

I would be interested in taking it up. clintontak22 are you still working on it?

Comment 6

[Caitlin Neiman [:caitmuenster]]

Hey Varun! Since we haven't heard from clintontak22, you're clear to pick this one up! Please needinfo :mixedpuppy if you need any help getting started.

Comment 7

[Varun [he/him IST]]

Attachment: Chrome empty default_icon error

Seems like Chrome throws an error if default_icon is empty or one of its key is empty. They also validate if the icon path is correct or not. manifest - https://pastebin.com/MjyQkKng

I think it seems the correct way to go.

Comment 8

[Varun [he/him IST]]

So I was thinking of putting a validation check for default_icons similar to chrome (throw an error if the file does not exist/is empty) but turns out Firefox does not validate file path for icons as well. Keeping validation check for default_icons but not for icons would look a bit weird IMO.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Assuming the problem is only "default_icon": ""...

It probably just needs to be handled in schema.  I think it could be done by creating a new NonEmptyExtensionUrl type that is the same as ExtensionUrl, but uses a pattern to ensure it is non-empty, then make IconPath use that.

I don't think we want to validate the icons exist during extension startup.  We probably only would want to do that during install, if at all.  Extension authors should be verifying that their extension works properly.

[1] https://searchfox.org/mozilla-central/rev/f8de61826903996f6bdf41b11a2844dd59ac144f/toolkit/components/extensions/schemas/manifest.json#543

Comment 10

[Varun [he/him IST]]

Attachment: Fix Bug 1419109 - Empty default icon shows blank icon in toolbar

If the extension had either default_icon or one of it's property as an empty string, it would show a black icon in the toolbar. With this patch, it checks if any of default_icon property is empty and throws an error on extension load.

Comment 11

[Varun [he/him IST]]

checkin-needed

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a3bbf490ea41
Fix Bug 1419109 - Empty default icon shows blank icon in toolbar r=mixedpuppy

Comment 13

[Cristina Coroiu [:ccoroiu]]

Backout for browser-chrome failures at browser/test-oop-extensions/browser_ext_browserAction_pageAction_icon_permissions.js

Backout: https://hg.mozilla.org/integration/autoland/rev/2bb5deb589500edcab98659c4a6659d9bf3e5ffa

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a3bbf490ea419b2c01a4447d45e7fe0a116144f1

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=221605988&repo=autoland&lineNumber=3217

task 2019-01-13T16:35:00.297Z] 16:35:00 INFO - TEST-PASS | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_pageAction_icon_permissions.js | pageAction image removed from document -
[task 2019-01-13T16:35:00.299Z] 16:35:00 INFO - Extension loaded
[task 2019-01-13T16:35:00.303Z] 16:35:00 INFO - Buffered messages finished
[task 2019-01-13T16:35:00.305Z] 16:35:00 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_pageAction_icon_permissions.js | Uncaught exception - startup failed
[task 2019-01-13T16:35:00.306Z] 16:35:00 INFO - Leaving test bound testDefaultDetails
[task 2019-01-13T16:35:00.308Z] 16:35:00 INFO - Entering test bound testSecureURLsDenied
[task 2019-01-13T16:35:00.311Z] 16:35:00 INFO - Extension loaded
[task 2019-01-13T16:35:00.313Z] 16:35:00 INFO - Console message: [JavaScript Error: "1547397299640 addons.webextension.<unknown> ERROR Loading extension 'null': Reading manifest: Error processing browser_action.default_icon: Value "/foo/bar.png" must either: be an object value, or match the pattern /^\w/" {file: "resource://gre/modules/Log.jsm" line: 679}]
[task 2019-01-13T16:35:00.319Z] 16:35:00 INFO - append@resource://gre/modules/Log.jsm:679:9
[task 2019-01-13T16:35:00.321Z] 16:35:00 INFO - log@resource://gre/modules/Log.jsm:360:7
[task 2019-01-13T16:35:00.323Z] 16:35:00 INFO - error@resource://gre/modules/Log.jsm:368:5
[task 2019-01-13T16:35:00.325Z] 16:35:00 INFO - _logMessage@resource://gre/modules/Extension.jsm:363:26
[task 2019-01-13T16:35:00.327Z] 16:35:00 INFO - logError@resource://gre/modules/Extension.jsm:359:5
[task 2019-01-13T16:35:00.330Z] 16:35:00 INFO - packagingError@resource://gre/modules/Extension.jsm:346:5
[task 2019-01-13T16:35:00.332Z] 16:35:00 INFO - manifestError@resource://gre/modules/Extension.jsm:332:5
[task 2019-01-13T16:35:00.336Z] 16:35:00 INFO - parseManifest@resource://gre/modules/Extension.jsm:601:7
[task 2019-01-13T16:35:00.338Z] 16:35:00 INFO - asyncloadManifest@resource://gre/modules/Extension.jsm:842:7
[task 2019-01-13T16:35:00.340Z] 16:35:00 INFO - async_receiveMessageAPI@chrome://mochikit/content/tests/SimpleTest/SpecialPowersObserverAPI.js:605:9
[task 2019-01-13T16:35:00.342Z] 16:35:00 INFO - ChromePowers.prototype._receiveMessage@chrome://mochikit/content/tests/SimpleTest/ChromePowers.js:81:14
[task 2019-01-13T16:35:00.344Z] 16:35:00 INFO - ChromePowers.prototype._sendAsyncMessage@chrome://mochikit/content/tests/SimpleTest/ChromePowers.js:43:3
[task 2019-01-13T16:35:00.345Z] 16:35:00 INFO - startup@chrome://mochikit/content/tests/SimpleTest/specialpowersAPI.js:2067:9
[task 2019-01-13T16:35:00.347Z] 16:35:00 INFO - testDefaultDetails@chrome://mochitests/content/browser/browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_pageAction_icon_permissions.js:102:24
[task 2019-01-13T16:35:00.349Z] 16:35:00 INFO - AsyncTester_execTest/<@chrome://mochikit/content/browser-test.js:1108:34
[task 2019-01-13T16:35:00.350Z] 16:35:00 INFO - asyncTester_execTest@chrome://mochikit/content/browser-test.js:1099:16
[task 2019-01-13T16:35:00.351Z] 16:35:00 INFO - nextTest/<@chrome://mochikit/content/browser-test.js:997:9
[task 2019-01-13T16:35:00.353Z] 16:35:00 INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:803:59
[task 2019-01-13T16:35:00.354Z] 16:35:00 INFO -
[task 2019-01-13T16:35:00.356Z] 16:35:00 INFO - Console message: [JavaScript Error: "Security Error: Content at moz-extension://4175c09b-edf1-49ee-9930-f31a0169d5b5/ may not load or link to chrome://browser/content/browser.xul."]
[task 2019-01-13T16:35:00.358Z] 16:35:00 INFO - Console message: [JavaScript Error: "Security Error: Content at moz-extension://4175c09b-edf1-49ee-9930-f31a0169d5b5/ may not load or link to chrome://browser/content/browser.xul."]

Comment 14

[Cristina Coroiu [:ccoroiu]]

Xpcshell failed too: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=221606369&repo=autoland&lineNumber=2167

Comment 15

[Shane Caraveo (:mixedpuppy)]

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7bf0d21448d
Fix Bug 1419109 - Empty default icon shows blank icon in toolbar r=mixedpuppy

Comment 17

[Eliza Balazs [:ebalazs_]]

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

Comment 18

[Caitlin Neiman [:caitmuenster]]

Thanks for the patch, Varun! 🎉 Your contribution has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition

Would you be interested in creating a profile on mozillians.org? I'd be happy to vouch for you!

Comment 19

[Varun [he/him IST]]

Thanks Caitlin. My mozilillians profile is https://mozillians.org/en-US/u/varundey/ :)

Comment 20

[Caitlin Neiman [:caitmuenster]]

You're vouched! Welcome onboard. I look forward to seeing you around!

Comment 21

[Vlad Jiman]

Verified that the error mentioned in comment #10 is shown if there is is no value in the "default_icon". I've used the same test extension to test if it installs successfully and throws the error having only edited the default_icon value. Testing was run on Firefox 66.0b3, 65 and Nightly 67.0a1.