Closed Bug 985614 Opened 6 years ago Closed 6 years ago

Packaged app installation does not trigger `onsuccess` callback

Categories

(Firefox for Android :: Web Apps (PWAs), defect, P1, major)

29 Branch
All
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 32
Tracking Status
firefox31 + fixed
firefox32 --- fixed
fennec 31+ ---

People

(Reporter: mstriemer, Assigned: mhaigh)

References

Details

(Whiteboard: [WebRuntime] [dependency: marketplace])

Attachments

(2 files, 3 obsolete files)

After successfully installing a packaged app with `navigator.mozApps.installPackage` on Android the registered `onsuccess` callback is not fired.

I CC'd mhaigh and myk since this is somewhat related to bug 978143.
I see this too when installing packaged apps from Marketplace.  I searched for ":packaged" to find packaged apps and then tapped the Free button for UFart, which was at the top of the list.  The Free button turned into a throbber, and I was prompted to install the app.  I tapped the Install button to install it and then tapped the "Done" button in the installation confirmation dialog to return to Fennec.

But the throbber never changed to a Launch button, and eventually it turned back into a Free button (presumably because Marketplace never received the "installation succeeded" event, and the throbber timed out).  Reloading the search page correctly displayed a Launch button next to UFart, however, so the app appears to have been installed into the registry properly, and Marketplace can see that it's installed.
Priority: -- → P1
Whiteboard: [WebRuntime]
Blocks: 993102
Having a similar issue with bug 975921. Do we have the app's events like 'downloaderror' documented anywhere?
Depends on: 975921
(In reply to Davor Spasovski [:spasovski] from comment #2)
> Having a similar issue with bug 975921. Do we have the app's events like
> 'downloaderror' documented anywhere?

I'm not sure "documented" is the right word, but they're specified in this XPIDL file:

https://github.com/mozilla/gecko-dev/blob/c3fc50772fd81ef05a1d27a9ec9897a41294b90f/dom/interfaces/apps/nsIDOMApplicationRegistry.idl#L62-L72


> Depends on: 975921

Did you mean to create this dependency?  It isn't clear how this bug depends on that one being fixed.
Flags: needinfo?(dspasovski)
No longer depends on: 975921
Flags: needinfo?(dspasovski)
Yea the dependency was my bad. Thanks for the reference.
It would be nice to fix this for Android and getting it in the next train, this is a bit of an issue.
Whiteboard: [WebRuntime] → [WebRuntime] [dependency: marketplace]
Assignee: nobody → mhaigh
Bumping priority, given the downstream impact.  Also, note upstream bug 997717, which seems related.
Severity: normal → major
I think the problem is here: http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#2519

The Webapps:Install:Return:OK isn't sent.
Remove superfluous autoinstall flag and check existence of requestId to determine if there's a UI to return a success message to
Attachment #8426947 - Flags: review?(wjohnston)
Attachment #8426947 - Flags: review?(myk)
Attachment #8426947 - Flags: review?(mark.finkle)
Comment on attachment 8426947 [details] [diff] [review]
trigger onsuccess for package install from marketplace

I think Myk knows this code well enough to be the prime reviewer. I would suggest skipping the mostly unused "headlessInstall" local and just using "!aData.request" directly. Unless you feel the variable gives a bit more context.
Attachment #8426947 - Flags: review?(wjohnston)
Attachment #8426947 - Flags: review?(mark.finkle)
Attachment #8426947 - Flags: feedback+
Removed the headless variable and added a comment for context
Attachment #8426947 - Attachment is obsolete: true
Attachment #8426947 - Flags: review?(myk)
Attachment #8427048 - Flags: review?(myk)
Comment on attachment 8427048 [details] [diff] [review]
trigger onsuccess for package install from marketplace

Review of attachment 8427048 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!  It also suggests that WebappManager.autoInstall is misleadingly named, as the method is shared by both the mozApps and the "sideloaded APK" install paths.  Not to mention that both methods require user confirmation of the install, so neither of them are quite "automatic".

But that's a semantic issue for another bug.  Carry on!

::: dom/apps/src/Webapps.jsm
@@ +2524,5 @@
>      yield this._saveApps();
>  
>      this.broadcastMessage("Webapps:AddApp", { id: id, app: appObject });
> +
> +    // the presence of a requestID means that we have a page to update

Yay for informative comments and complete sentences!  But nit: use initial-letter capitalization and punctuation to delimit such complete sentences, as with the comment below this one.
Attachment #8427048 - Flags: review?(myk) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/52632d896ef7
Keywords: checkin-needed
Whiteboard: [WebRuntime] [dependency: marketplace] → [WebRuntime] [dependency: marketplace][fixed-in-fx-team]
Whiteboard: [WebRuntime] [dependency: marketplace][fixed-in-fx-team] → [WebRuntime] [dependency: marketplace]
I think this is failing because OperatorApps::_launchInstall calls confirmInstall without a requestID too.
We used to go in the |else| branch, instead we're now going in the |if|.
We're also calling onInstallSuccessAck twice in that case (because forceSuccessAck is true).
Have added an apk specific flag which should prevent the issue as described by Marco.

https://tbpl.mozilla.org/?tree=Try&rev=24a8d453e327
Attachment #8427702 - Attachment is obsolete: true
Attachment #8433290 - Flags: feedback?(myk)
Attachment #8433290 - Flags: feedback?(mar.castelluccio)
Flags: needinfo?(mhaigh)
Comment on attachment 8433290 [details] [diff] [review]
trigger onsuccess for package install from marketplace

This looks like a good minimal fix.  In the long term, I'll want to revisit this, as I'm uneasy about the way we take this shortcut directly to onInstallSuccessAck in only some cases.  But in the meantime we should do so correctly, and this fix makes the code do that.
Attachment #8433290 - Flags: feedback?(myk) → feedback+
Comment on attachment 8433290 [details] [diff] [review]
trigger onsuccess for package install from marketplace

Erm, actually, I can review code in Webapps.jsm now, so r=myk.
Attachment #8433290 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/35a50dd7629d
Keywords: checkin-needed
Whiteboard: [WebRuntime] [dependency: marketplace] → [WebRuntime] [dependency: marketplace][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/35a50dd7629d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [WebRuntime] [dependency: marketplace][fixed-in-fx-team] → [WebRuntime] [dependency: marketplace]
Target Milestone: --- → Firefox 32
Comment on attachment 8433290 [details] [diff] [review]
trigger onsuccess for package install from marketplace

Review of attachment 8433290 [details] [diff] [review]:
-----------------------------------------------------------------

My feedback isn't needed anymore, but it looks good to me ;)
Attachment #8433290 - Flags: feedback?(mar.castelluccio)
Here's a patch for uplift to 31, but this fix exposes bug 1021443, which was caused by bug 989046, which landed in 31.  So we'll need to uplift that fix too, once it lands.

Thus I'm not yet setting an approval-mozilla-* flag, but here's the approval request comment once we're ready for uplift:

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Bug 934756.

User impact if declined: 
  Users who install packaged apps from Marketplace will not be able to launch
  them from Marketplace right after installing them, as the install button will
  never change to a Launch button.

  Also, Marketplace analytics will record packaged app install attempts as
  unsuccessful, making the success rate look much lower than it actually is.

Testing completed (on m-c, etc.): 
  This has been on Central for a couple days.

Risk to taking this patch (and alternatives if risky): 
  It's the low-risk, obvious fix.

String or IDL/UUID changes made by this patch:
  None.
Noting uplift dependency.
tracking-fennec: --- → 31+
Depends on: 1021443
Hardware: ARM → All
Tracking. We want that in 31 because it will impact most of the users of the marketplace.
Comment on attachment 8436174 [details] [diff] [review]
patch for uplift to Fx31

See comment 22 for the Approval Request Comment!
Attachment #8436174 - Flags: approval-mozilla-beta?
Comment on attachment 8436174 [details] [diff] [review]
patch for uplift to Fx31

Erm, actually the uplift of blocking bug 1021443 was reverted, so cancelling this uplift request until that one is relanded.
Attachment #8436174 - Flags: approval-mozilla-beta?
I get "Launch" for packaged apps with this patch alone applied to Beta.

I do see this in the log:

I/InstallAppProgress( 6830): Finished installing com.firefox.marketplace.p1d5cf395eb02b6140b924a249a914ba1
E/GeckoWebappEventListener( 6599): error unregistering install receiver:
E/GeckoWebappEventListener( 6599): java.lang.IllegalArgumentException: Receiver not registered: org.mozilla.gecko.webapp.InstallListener@4209a168
E/GeckoWebappEventListener( 6599): 	at android.app.LoadedApk.forgetReceiverDispatcher(LoadedApk.java:658)
E/GeckoWebappEventListener( 6599): 	at android.app.ContextImpl.unregisterReceiver(ContextImpl.java:1461)
E/GeckoWebappEventListener( 6599): 	at android.content.ContextWrapper.unregisterReceiver(ContextWrapper.java:489)
E/GeckoWebappEventListener( 6599): 	at org.mozilla.gecko.webapp.EventListener$2.onActivityResult(EventListener.java:235)
E/GeckoWebappEventListener( 6599): 	at org.mozilla.gecko.ActivityHandlerHelper.handleActivityResult(ActivityHandlerHelper.java:33)
E/GeckoWebappEventListener( 6599): 	at org.mozilla.gecko.GeckoApp.onActivityResult(GeckoApp.java:2419)


so something like the opposite of Bug 1021443. Myk?
Comment on attachment 8436174 [details] [diff] [review]
patch for uplift to Fx31

(In reply to Richard Newman [:rnewman] from comment #27)
> I get "Launch" for packaged apps with this patch alone applied to Beta.

Woot!  Let's request uplift to Beta.  See comment 22 for the Approval Request Comment.


> I do see this in the log:> so something like the opposite of Bug 1021443. Myk?

That's bug 974578, and fortunately it's harmless (besides spamming the log and looking scary), so we won't need to uplift its fix.
Attachment #8436174 - Flags: approval-mozilla-beta?
Attachment #8436174 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.