1845691 - catch ("Additional precaution to be entirely sure that we cannot reject.") does not work

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, defect)

Description

[Rob Wu [:robwu]]

There is a promise variable that is expected to settle and set isPhaseEnd to true at https://searchfox.org/mozilla-central/rev/bf272a7be6fef0e91bde01dfe1f68403d8fbc237/toolkit/components/asyncshutdown/AsyncShutdown.sys.mjs#563-565,570,575,577

      promise = ...
        .catch
        // Additional precaution to be entirely sure that we cannot reject.
        ();
    ...
    promise.then(() => (isPhaseEnd = true)); // This promise cannot reject

The above was added in bug 1249590 in a clear attempt to "bullet-proof" it, but does not work, because .catch() without a function parameter does not swallow the error. Consequently, it is possible for isPhaseEnd to never be set to true. This might be a cause of bug 1442971.

As a comparable isolated test case, just run Promise.reject(1).catch() from the console and you'll see that the result is still a rejected promise.

The corrected version should pass a function (even a dummy one) or to use promise.finally instead of .catch() + .promise.then.

Comment 1

[Rob Wu [:robwu]]

Attachment: Bug 1845691 - Fix broken catch() in AsyncShutdown

Comment 2

[Rob Wu [:robwu]]

I looked up other potential callers and filed bug 1845695 as a follow-up for another suspicious case.

There is also this useless .catch(), but due to an earlier .catch(ex => ex) it doesn't break anything: https://searchfox.org/mozilla-central/rev/00e6644d0db8acf9372702324151b8077a3d2bb7/browser/components/newtab/content-src/components/Card/Card.jsx#64,66

Comment 3

[:Gijs (he/him)]

(In reply to Rob Wu [:robwu] from comment #2)

I looked up other potential callers and filed bug 1845695 as a follow-up for another suspicious case.

Could we have a linting rule to, uh, catch this, in future? :-)

There is also this useless .catch(), but due to an earlier .catch(ex => ex) it doesn't break anything: https://searchfox.org/mozilla-central/rev/00e6644d0db8acf9372702324151b8077a3d2bb7/browser/components/newtab/content-src/components/Card/Card.jsx#64,66

Comment 4

[Rob Wu [:robwu]]

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

(In reply to Rob Wu [:robwu] from comment #2)

I looked up other potential callers and filed bug 1845695 as a follow-up for another suspicious case.

Could we have a linting rule to, uh, catch this, in future? :-)

I filed bug 1845725 for an eslint rule to catch catch calls

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/95c00823b834
Fix broken catch() in AsyncShutdown r=Gijs

Comment 6

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/95c00823b834