1724718 - Also MOZ_CRASH for missing files that go through CreateLocalJarInput helper

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Gijs (he/him)]

Andrei reported an issue with the crashes from bug 1721627 that happened locally but not on infra. When narrowing that down, I realized that I missed a JAR helper that gets called off-mainthread. Let's fix that. There's probably more brokenness lurking here. I think we've missed all async calls on jar: channels so far, which is likely to include missing images in various tests...

Comment 1

[:Gijs (he/him)]

https://treeherder.mozilla.org/jobs?repo=try&revision=ad7fa2843b080376d707a6ed284ee8a0e867f88c

Comment 2

[:Gijs (he/him)]

Luca or Mark, the trypush is flagging up this:

https://searchfox.org/mozilla-central/rev/f71cb98fc35da418d2cb9ce31a0416d532dc9d69/toolkit/mozapps/extensions/content/abuse-report-panel.js#755

returns undefined in some cases, which means we try to load chrome://mozapps/content/extensions/undefined, which of course doesn't exist.

(from local running, the JS stack looks like this:

0 update() ["chrome://mozapps/content/extensions/abuse-report-panel.js":618:22]
    this = [object HTMLElement]
1 setAbuseReport(abuseReport = "[object Object]") ["chrome://mozapps/content/extensions/abuse-report-panel.js":642:11]
    this = [object HTMLElement]
2 <TOP LEVEL> ["chrome://mozapps/content/extensions/abuse-report-panel.js":864:5]

Is it expected the iconURL or add-on can be missing here sometimes, and in that case, should we load nothing or a fallback image of some sort? Or are both the add-on and its iconURL guaranteed to exist, in which case, should we just fix the test so it passes a realistic object, and remove the && that copes with the addon being null/undefined?

Comment 3

[:Gijs (he/him)]

Attachment: Bug 1724718 - fix various trivial dead references in product code, r?jaws,mossop,mconley

Comment 4

[:Gijs (he/him)]

Attachment: Bug 1724718 - do not set an image attribute for synced tabs items in the synced tabs menupanel unless there is an icon to show, r?mconley,markh

Depends on D122162

Comment 5

[:Gijs (he/him)]

Attachment: Bug 1724718 - do not rely on images that only exist on macOS in devtools tests and fix MenuItem dummy image, r?mtigley

Depends on D122163

Comment 6

[:Gijs (he/him)]

Attachment: Bug 1724718 - work around avatar URL issues in test_fxaccounts_button.html, r?tgiles

Depends on D122164

Comment 7

[:Gijs (he/him)]

Attachment: Bug 1724718 - forget AddTask.js ever existed as bug 1544051 already removed it, r?bgrins

Depends on D122165

Comment 8

[:Gijs (he/him)]

Attachment: Bug 1724718 - put missing chrome/resource check from jar channels in a better place so we catch all cases, r?valentin,florian!

Depends on D122166

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9d3cf125f605
do not set an image attribute for synced tabs items in the synced tabs menupanel unless there is an icon to show, r=markh

Comment 10

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

Attachment: Bug 1724718 - Abuse Report panel should fallback to extensionGeneric.svg on addons without an iconURL. r?gijs!

Some addons may not include an icon (e.g. static themes addons) and so the abuse report panel should
fallback to the extensionGeneric.svg.

Falling back to the extensionGeneric.svg is also needed to prevent a diagnostic crash triggered by
loading non existing resources while running the abuse report mochitests.

Comment 11

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

(In reply to :Gijs (he/him) from comment #2)

Luca or Mark, the trypush is flagging up this:

https://searchfox.org/mozilla-central/rev/f71cb98fc35da418d2cb9ce31a0416d532dc9d69/toolkit/mozapps/extensions/content/abuse-report-panel.js#755

returns undefined in some cases, which means we try to load chrome://mozapps/content/extensions/undefined, which of course doesn't exist.

(from local running, the JS stack looks like this:

0 update() ["chrome://mozapps/content/extensions/abuse-report-panel.js":618:22]
    this = [object HTMLElement]
1 setAbuseReport(abuseReport = "[object Object]") ["chrome://mozapps/content/extensions/abuse-report-panel.js":642:11]
    this = [object HTMLElement]
2 <TOP LEVEL> ["chrome://mozapps/content/extensions/abuse-report-panel.js":864:5]

Is it expected the iconURL or add-on can be missing here sometimes, and in that case, should we load nothing or a fallback image of some sort? Or are both the add-on and its iconURL guaranteed to exist, in which case, should we just fix the test so it passes a realistic object, and remove the && that copes with the addon being null/undefined?

Hi Gijs,
I just looked into that and, even if the test is triggered by mock addons installed through the MockProvider (defined in head_abusereport.js, and I they do not have an iconURL), the scenario is actually legit: e.g. the static themes addons usually do not have an icon (in about:addons we do show a screenshot preview of the theme, but that doesn't fit in the current abuse report panel UI).

When the icon URL is missing, it would be reasonable to fallback to the extensionGeneric.svg file (as we do in some other parts of the UI when we have to show an addon icon and the addon does not have one).

I've attached a patch for doing that in D122207 (and verified locally that with this change browser_html_abuse_report.js doesn't trigger the MOZ_CRASH anymore as expected, and the generic icon seems to fit well in both dark and light themes, by being white when the abuse report panel is dark and black when the abuse report panel has a light white background).

Feel free to add the patch to your stack as is, or to commandeer the patch if you prefer.

(and let me know if there is any other addons-related test that is triggering this diagnostic crash, and I'll take a look asap).

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9d5d2dc82665
fix various trivial dead references in product code, r=mossop

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e4e3098eaf56
Abuse Report panel should fallback to extensionGeneric.svg on addons without an iconURL. r=Gijs

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f75f84ad84a5
work around avatar URL issues in test_fxaccounts_button.html, r=tgiles

Comment 15

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/9d3cf125f605
https://hg.mozilla.org/mozilla-central/rev/9d5d2dc82665
https://hg.mozilla.org/mozilla-central/rev/e4e3098eaf56
https://hg.mozilla.org/mozilla-central/rev/f75f84ad84a5

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc5556739c43
forget AddTask.js ever existed as bug 1544051 already removed it, r=bgrins

Comment 17

[Atila Butkovits]

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/675fd489778c
do not rely on images that only exist on macOS in devtools tests and fix MenuItem dummy image, r=mtigley

Comment 19

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/675fd489778c

Comment 20

[:Gijs (he/him)]

Attachment: Bug 1724718 - fake global.css for android reftests and crashtests, r?emilio

Comment 21

[:Gijs (he/him)]

Attachment: Bug 1724718 - skip some XUL-y tests on android, r?emilio!,agi!

Depends on D122663

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c1f695ba3371
fake global.css for android reftests and crashtests, r=emilio
https://hg.mozilla.org/integration/autoland/rev/aaecf2012cb0
skip some XUL-y tests on android, r=emilio,agi

Comment 23

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/c1f695ba3371
https://hg.mozilla.org/mozilla-central/rev/aaecf2012cb0

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7fa060b566d6
put missing chrome/resource check from jar channels in a better place so we catch all cases, r=valentin

Comment 25

[Iulian Moraru]

Backed out for causing failures on nsJARChannel.

Push with failures

Failure log for a11y failures
Failure log for gv-junit failures
Failure log for gv-junit-fis failures
Failure log for gv-junit-e10s-single failures
Failure log for talos other failures

Backout link

Comment 26

[:Gijs (he/him)]

(In reply to Iulian Moraru from comment #25)

Backed out for causing failures on nsJARChannel.

Push with failures

Failure log for a11y failures
Failure log for gv-junit failures
Failure log for gv-junit-fis failures
Failure log for gv-junit-e10s-single failures

Backout link

Huh, clearly it's the tests you didn't know existed that get you - I ran what I thought would have been a pretty comprehensive trypush before landing: https://treeherder.mozilla.org/jobs?repo=try&revision=0372dbe2d3c9f31365675213d5ba0b52a47ad587

I'll look at these some more, but I'm out on PTO for most of the next 4 weeks so it may take me a while...

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/194aa5b57fa7
put missing chrome/resource check from jar channels in a better place so we catch all cases, r=valentin,florian,necko-reviewers

Comment 28

[Marian-Vasile Laza]

Backed out changeset 194aa5b57fa7 (Bug 1724718) for causing wpt failures on report-frame-ancestors-with-x-frame-options.sub.html and multiple other.
Backout link
Push with failures <--> wpt17
Failure Log
Also c1 Failure Log
Also wpt32 Failure Log
Also 3 Failure Log
Also C Failure Log
Also wpt16 Failure Log
Also 4 Failure Log
...

Comment 29

[:Gijs (he/him)]

Ugh. Seems I need a more extensive try. Let's give that a shot: https://treeherder.mozilla.org/jobs?repo=try&revision=8027b3db6c1aa740712d00d9b8e79eb9ea292259

Comment 30

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #29)

Ugh. Seems I need a more extensive try. Let's give that a shot: https://treeherder.mozilla.org/jobs?repo=try&revision=8027b3db6c1aa740712d00d9b8e79eb9ea292259

... it'd help if it was (a) not an artifact try, and (b) on a more recent central. Let's try again: https://treeherder.mozilla.org/jobs?repo=try&revision=577591dbc57cb3bcb3acf9ca930618b4ae7eef8e

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e939a80b5b75
put missing chrome/resource check from jar channels in a better place so we catch all cases, r=valentin,florian,necko-reviewers

Comment 32

[:Gijs (he/him)]

Attachment: WIP: Bug 1724718 - follow-up: add exemptions for more android-specific missing files.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/027ee4c52450
follow-up: add exemptions for more android-specific missing files. CLOSED TREE

Comment 34

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/e939a80b5b75
https://hg.mozilla.org/mozilla-central/rev/027ee4c52450