empty default icon in manifest produces blank icon in toolbar

VERIFIED FIXED in Firefox 66

Status

P5
normal
VERIFIED FIXED
a year ago
2 months ago

People

(Reporter: mixedpuppy, Assigned: vdey, Mentored)

Tracking

({good-first-bug})

58 Branch
mozilla66
good-first-bug

Firefox Tracking Flags

(firefox66 verified, firefox67 verified)

Details

Attachments

(2 attachments)

(Reporter)

Description

a year ago
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.
Mentor: mixedpuppy

Comment 1

a year ago
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

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

Updated

9 months ago
Product: Toolkit → WebExtensions
(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
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.
Flags: needinfo?(mixedpuppy)
(Assignee)

Comment 5

3 months ago
I would be interested in taking it up. clintontak22 are you still working on it?
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.
Assignee: nobody → varundey20
Flags: needinfo?(mixedpuppy)
(Assignee)

Comment 7

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

Updated

3 months ago
Flags: needinfo?(mixedpuppy)
(Assignee)

Comment 8

3 months ago
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.
Flags: needinfo?(mixedpuppy)
(Assignee)

Updated

3 months ago
Flags: needinfo?(mixedpuppy)
(Reporter)

Comment 9

3 months ago
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
Flags: needinfo?(mixedpuppy)
(Assignee)

Comment 10

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

Comment 11

2 months ago

checkin-needed

Keywords: checkin-needed

Comment 12

2 months ago

Pushed by dvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a3bbf490ea41
Fix Bug 1419109 - Empty default icon shows blank icon in toolbar r=mixedpuppy

Keywords: checkin-needed

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 - async
Tester_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."]

Flags: needinfo?(varundey20)
(Assignee)

Updated

2 months ago
Flags: needinfo?(varundey20)
Keywords: checkin-needed

Comment 16

2 months ago

Pushed by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d7bf0d21448d
Fix Bug 1419109 - Empty default icon shows blank icon in toolbar r=mixedpuppy

Keywords: checkin-needed

Comment 17

2 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox66: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

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!

(Assignee)

Comment 19

2 months ago

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

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

Updated

2 months ago
Depends on: 1522209

Comment 21

2 months ago

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.

Status: RESOLVED → VERIFIED
status-firefox66: fixed → verified
status-firefox67: --- → verified
You need to log in before you can comment on or make changes to this bug.