1773800 - [Colorway Closet] Do not render colorways with missing icons

Status: RESOLVED FIXED

(Firefox :: Theme, task)

Description

[kpatenio]

This was first brought up in this patch. We observe a crash when selecting the Try Colorways button and loading a theme without an associated icon that we'd expect to show in the modal. (Ex. a mock theme for mochitests).

Hit MOZ_CRASH(Missing chrome or resource URLs: chrome://browser/content/null) at /builds/worker/checkouts/gecko/netwerk/base/nsNetUtil.cpp:3882

That is triggered if a monochromatic theme doesn't have an icon, which was the case for the test theme installed from the test case, the line that triggers the crash is this part of ColorwaySelector.prototype.render (where we are unconditionally setting the style property --colorway-icon to url(${theme.iconURL) even if iconURL is null, and that is translated into "chrome://browser/content/null" and trigger the crash).

The crash was triggered in both optimized and debug build while running the test, but it is restricted to while running in automation and so it shouldn't affect actual users, but I though to still point it out to you because we could also decide to avoid that by tweaking ColorwaySelector.prototype.render to either not set the --colorway-icon property if theme.iconURL is not set, or to skip the theme all together (and maybe logging an error in the browser console to point out that a certain theme id was skipped).

Comment 1

[kpatenio]

Attachment: WIP: Bug 1773800 - run browser_colorwaycloset_aboutaddons.js for nightly only. r=dao!

Comment 2

[kpatenio]

Attachment: Bug 1773800 - do not render Colorway in closet modal if there is no icon available. r=dao!

Comment 3

[Phabricator Automation]

Comment on attachment 9280788 [details]
WIP: Bug 1773800 - run browser_colorwaycloset_aboutaddons.js for nightly only. r=dao!

Revision D148969 was moved to bug 1773706. Setting attachment 9280788 [details] to obsolete.

Comment 4

[Sandor Molnar[:smolnar]]

This patch is not working in latest beta-sim

Also the perma crash is still present

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Sandor Molnar from comment #4)

This patch is not working in latest beta-sim

Also the perma crash is still present

based on a quick look to the logs nearby the crash here, it seems to be triggered because it can't find resource:///modules/ColorwayClosetOpener.jsm:

[task 2022-06-12T13:02:29.067Z] 13:02:29     INFO - PROCESS-CRASH | toolkit/mozapps/extensions/test/browser/browser_colorwaycloset_aboutaddons.js | application crashed [@ CheckForBrokenChromeURL(nsILoadInfo*, nsIURI*)]
[task 2022-06-12T13:02:29.067Z] 13:02:29     INFO - Mozilla crash reason: Missing chrome or resource URLs: resource:///modules/ColorwayClosetOpener.jsm
[task 2022-06-12T13:02:29.067Z] 13:02:29     INFO - Crash dump filename: /var/folders/3d/mrj8hrbx2m55zc0yxx18btdw000014/T/tmpdbm7fxjp.mozrunner/minidumps/FA2BA8BD-1032-4639-AA28-7DA82D75A251.dmp

I haven't double-checked explicitly why ColorwayClosetOpener.jsm isn't found while running the beta-simulation, but I thought to mention it right away because it is likely a useful hint for what to look into.

Comment 6

[kpatenio]

A patch for Bug 1773706 has recently been queued to land and will only run browser_colorwaycloset_aboutaddons.js under Nightly. (Also see this comment.) A separate ticket (Bug 1774042) has been filed to remove the restriction once we ship the new Colorway closet.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:dao, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 8

[kpatenio]

This fix was integrated in the patch for Bug 1747229, which was recently landed. Once that bug is closed, so can this one.

Comment 9

[kpatenio]

Resolving this bug as fixed since Bug 1747229 landed