1629952 - Throw when try to `cancel` an `INSTALLED` install

Status: RESOLVED FIXED

(GeckoView :: Extensions, defect, P1)

Description

[Christian Sadilek [:csadilek]]

When cancelling the GeckoResult provided from runtime.webExtensionController.install GV may crash.

The result returned from the cancel operation resolves to true and completes the result returned by install. However, cancellation didn't work, the extension is installed and sendSuccess subsequently causes a crash ("result already complete"):

2020-04-14 11:51:37.300 23870-23870/org.mozilla.samples.browser E/AndroidRuntime: FATAL EXCEPTION: main
    Process: org.mozilla.samples.browser, PID: 23870
    java.lang.IllegalStateException: result is already complete
        at org.mozilla.geckoview.GeckoResult.complete(GeckoResult.java:776)
        at org.mozilla.geckoview.WebExtensionController$WebExtensionResult.sendSuccess(WebExtensionController.java:422)
        at org.mozilla.gecko.EventDispatcher$JavaCallbackDelegate$1.run(EventDispatcher.java:452)
        at android.os.Handler.handleCallback(Handler.java:883)
        at android.os.Handler.dispatchMessage(Handler.java:100)
        at android.os.Looper.loop(Looper.java:214)
        at android.app.ActivityThread.main(ActivityThread.java:7356)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:492)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:930)

Comment 1

[Edd]

(In reply to Christian Sadilek [:csadilek] from comment #0)

When cancelling the GeckoResult provided from runtime.webExtensionController.install GV may crash.

The result returned from the cancel operation resolves to true and completes the result returned by install. However, cancellation didn't work, the extension is installed and sendSuccess subsequently causes a crash ("result already complete"):

2020-04-14 11:51:37.300 23870-23870/org.mozilla.samples.browser E/AndroidRuntime: FATAL EXCEPTION: main
    Process: org.mozilla.samples.browser, PID: 23871
    java.lang.IllegalStateException: result is already complete
        at org.mozilla.geckoview.GeckoResult.complete(GeckoResult.java:776)
        at org.mozilla.geckoview.WebExtensionController$WebExtensionResult.sendSuccess(WebExtensionController.java:422)
        at org.mozilla.gecko.EventDispatcher$JavaCallbackDelegate$1.run(EventDispatcher.java:452)
        at android.os.Handler.handleCallback(Handler.java:883)
        at android.os.Handler.dispatchMessage(Handler.java:100)
        at android.os.Looper.loop(Looper.java:214)
        at android.app.ActivityThread.main(ActivityThread.java:7356)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:492)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:930)

Comment 2

[:severin Rudie]

I think I was accidentally tagged here (this is well outside my wheelhouse). If you do need information from me, please lmk what I can help with.

Comment 3

[Edd]

(In reply to Edd from comment #1)

(In reply to Christian Sadilek [:csadilek] from comment #0)

When cancelling the GeckoResult provided from runtime.webExtensionController.install GV may crash.

The result returned from the cancel operation resolves to true and completes the result returned by install. However, cancellation didn't work, the extension is installed and sendSuccess subsequently causes a crash ("result already complete"):

2020-04-14 11:51:37.300 23870-23870/org.mozilla.samples.browser E/AndroidRuntime: FATAL EXCEPTION: main
    Process: org.mozilla.samples.browser, PID: 23871
    java.lang.IllegalStateException: result is already complete
        at org.mozilla.geckoview.GeckoResult.complete(GeckoResult.java:776)
        at org.mozilla.geckoview.WebExtensionController$WebExtensionResult.sendSuccess(WebExtensionController.java:422)
        at org.mozilla.gecko.EventDispatcher$JavaCallbackDelegate$1.run(EventDispatcher.java:452)
        at android.os.Handler.handleCallback(Handler.java:883)
        at android.os.Handler.dispatchMessage(Handler.java:100)
        at android.os.Looper.loop(Looper.java:214)
        at android.app.ActivityThread.main(ActivityThread.java:7356)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:492)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:930)

Comment 4

[[ex-Mozilla] Agi Sferro | :agi]

Looked into this. Looks like calling cancel() when the addon is fully installed does not throw an exception, while our code expects it to.

Talked with the extension team, looks like we can change the behavior to throw an exception in this case instead.

https://searchfox.org/mozilla-central/rev/dc4560dcaafd79375b9411fdbbaaebb0a59a93ac/toolkit/mozapps/extensions/internal/XPIInstall.jsm#1388-1410

Comment 5

[Shane Caraveo (:mixedpuppy)]

As discussed on matrix, the fast path to deal with this will be in geckoview. There is other code that uses the code path in comment 4 so changing it to throw is probably not correct, at least at this time.

Comment 6

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1629952 - Remove legacy addon install.cancel() support.

This patch removes some old code for non-restartless addons. When the code was
written, it was still possible to call cancel() on an addon after the addon
was installed and before it had persisted in the database.

This case is not possible anymore, and calling cancel() on a
STATE_INSTALLED install object doesn't actually do much (but makes the
calling code think it actually canceled successfully, since it doesn't throw).

There is one spot where we still keep around the install object because we
don't call _cleanup(), i.e. when installing an already present disabled
addon. For this case, we can just call _cleanup() at the end of the
enablement process.

There are quite a few tests that exploit calling cancel() to mean
uninstall(), for those actually calling uninstall() works fine.

This fixes a GeckoView bug where calling cancel() doesn't throw (so GV thinks
the cancellation went through successfully) but then we receive an addon object
from the successfull installation.

Comment 7

[[ex-Mozilla] Agi Sferro | :agi]

Green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe69b899f13d9a033a6e65c2352042f91e5c1a30

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/786017cc292d
Remove legacy addon install.cancel() support. r=aswan,mixedpuppy

Comment 9

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

These are current, open bugs with a Severity of critical. The Severity of these bugs is being changed to S2 to be consistent with the May 4 2020 Severity definitions.

Please let Release Management know if these bugs are still S2.

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/786017cc292d

Comment 11

[Christian Sadilek [:csadilek]]

Unfortunately, we can still reproduce this problem. It's improved now, but we still see the original crash upon cancellation. A reliable way to reproduce the problem is to install/cancel the same add-on repeatedly.

STR:

• Install add-on
• Cancel
• Install same add-on
• Cancel again

Comment 12

[[ex-Mozilla] Agi Sferro | :agi]

The issue in Comment 11 is different from the one in this bug so I opened a meta to track all of them: Bug 1637766