Closed Bug 982559 Opened 7 years ago Closed 7 years ago

updating to Synthetic APKs and tapping Home screen icon for previously-installed webapp crashes Fennec

Categories

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

30 Branch
ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox29 + verified
firefox30 + verified
firefox31 --- verified

People

(Reporter: myk, Assigned: myk)

Details

(Whiteboard: [WebRuntime])

Attachments

(2 files, 1 obsolete file)

Updating Fennec to a version that supports Synthetic APKs and then tapping a Home screen icon for an existing webapp causes Fennec to crash with an uncaught NullPointerException.


Steps to Reproduce:

1. install a version of Fennec without Synthetic APKs;
2. install an app;
3. update Fennec to a version with Synthetic APKs;
4. tap the Home screen icon for the app.

Expected Results: unsure; perhaps Fennec should tell you that you have to update the app (and offer to help you do it).  Regardless, Fennec shouldn't crash.

Actual Results: Fennec crashes.


First, logcat reports:

  782         ActivityManager  I  START u0 {act=org.mozilla.gecko.WEBAPP0 dat=http://cupcakes-vs-veggies.tresensa.com/manifest.webapp flg=0x10000000 cmp=org.mozilla.fennec_myk/.WebApps$WebApp0 bnds=[12,1152][276,1451]} from pid 12103
  5571        GeckoWebappImpl  W  Can't find package name for webapp
  5571        GeckoWebappImpl  E  Can't find package for webapp null
  5571        GeckoWebappImpl  E  android.content.pm.PackageManager$NameNotFoundException
  5571        GeckoWebappImpl  E  at android.app.ApplicationPackageManager.getApplicationInfo(ApplicationPackageManager.java:227)
…

Then it reports:

  5571          GeckoAppShell  E  >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1 ("main")
  5571          GeckoAppShell  E  java.lang.NullPointerException
  5571          GeckoAppShell  E  at org.mozilla.gecko.webapp.WebappImpl.onCreate(WebappImpl.java:109)
…
(In reply to Myk Melez [:myk] [@mykmelez] from comment #0)
> Expected Results: unsure; perhaps Fennec should tell you that you have to
> update the app (and offer to help you do it).  Regardless, Fennec shouldn't
> crash.

I agree with the shouldn't crash part, yes. What is our intended migration strategy, if any, for the interim preliminary release?
(In reply to Aaron Train [:aaronmt] from comment #1)
> I agree with the shouldn't crash part, yes. What is our intended migration
> strategy, if any, for the interim preliminary release?

Or intended strategy is to communicate to users the need to migrate their apps manually if they previously installed and are currently using apps installed via the old implementation.  We considered implementing an automated migrator (bug 957060) but decided that it isn't an MVP blocker.
Priority: -- → P1
Whiteboard: [WebRuntime]
Attached patch work in progress (obsolete) — Splinter Review
This seems to work, but I need to do more testing and clean up the code.
Assignee: nobody → myk
Status: NEW → ASSIGNED
Here's a robust patch, but I still want to do more testing before I request review, as I don't fully understand all the ways shortcuts used to be created.
Attachment #8395035 - Attachment is obsolete: true
Comment on attachment 8395287 [details] [diff] [review]
patch v1: use intent data to start app from legacy shortcut

Ok, I did more testing on a variety of hosted and packaged apps, and I'm satisfied that this patch will enable users to launch apps installed as shortcuts with a newer version of Firefox that supports synthetic APKs, both before and after those apps are updated with APKs.

Note that the patch doesn't actually migrate those shortcuts to homescreen icons for APK-based apps.  Users will still have to do that manually.  But it does enable the shortcuts to work in the meantime.  And they refer to the same apps.

In the process, I tried to guard against any such crashes in the future by conditionally accessing ApkResources in a variety of places only if the app was started via an APK.  And I fixed two other problems I noticed:

1. Code paths that canceled the intent and called finish() because of an error didn't return early, so they would continue to execute code in WebappImpl.onCreate that depended on those errors not occurring.

To fix this, I made those code paths return early after calling finish().


2. The code that sets mOrigin in WebappImpl.launchWebapp seems to have been copied almost verbatim from the old WebappImpl, but it executes at the end of a method in the old implementation, where it doesn't affect launching the webapp, even if it returns early.

In the new implementation, however, it was placed at the beginning of launchWebapp, and if it returns early, then launchWebapp never launches the webapp.

To fix this, I moved the code into a setOrigin method that launchWebapp calls, where an early return doesn't affect launching the webapp.
Attachment #8395287 - Flags: review?(wjohnston)
Comment on attachment 8395287 [details] [diff] [review]
patch v1: use intent data to start app from legacy shortcut

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

LGTM.

::: mobile/android/base/webapp/WebappImpl.java
@@ +98,5 @@
> +            }
> +            mManifestUrl = data.toString();
> +
> +            String shortcutName = extras.getString(Intent.EXTRA_SHORTCUT_NAME);
> +            mAppName = shortcutName != null ? shortcutName : "Web App";

Localize this string.

@@ +177,5 @@
> +
> +        // If this is a legacy shortcut, then we should have been able to get
> +        // the URI from the intent data.  Otherwise, we should have been able
> +        // to get it from the APK resources.  So we should never get here.
> +        Log.wtf(LOGTAG, "couldn't get URI from intent nor APK resources");

We try to use sentences in log messages. i.e. capital C in "couldn't"

@@ +346,5 @@
>                  return;
>              }
>  
> +            // If that failed fall back to the origin stored in the shortcut.
> +            // XXX Perhaps we should only do this if !mIsApk?

Probably yes.
Attachment #8395287 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #6)

> > +            mAppName = shortcutName != null ? shortcutName : "Web App";
> 
> Localize this string.

wesj and I discussed this on IRC and agreed to do it in a followup, since this string wasn't localizable in the old implementation, from which I copied this code:

http://hg.mozilla.org/mozilla-central/annotate/6fa163ff81a3/mobile/android/base/WebappImpl.java#l60

I'll file a followup bug once this lands.


> > +        // to get it from the APK resources.  So we should never get here.
> > +        Log.wtf(LOGTAG, "couldn't get URI from intent nor APK resources");
> 
> We try to use sentences in log messages. i.e. capital C in "couldn't"

This isn't a complete sentence, but perhaps the convention is still to use "sentence case"?  In any case, I've made the change!

https://github.com/mykmelez/gecko-dev/commit/a757f42


> > +            // If that failed fall back to the origin stored in the shortcut.
> > +            // XXX Perhaps we should only do this if !mIsApk?
> 
> Probably yes.

I checked the APK Factory library to ensure it doesn't set the intent's "data", and it doesn't:

https://github.com/mozilla/apk-factory-library/blob/e47d58e34f7cda07f3ef4d9ee21ca61961e4bec6/src/org/mozilla/android/synthapk/LauncherActivity.java#L43-L61

So this should indeed be only if !mIsApk.  Thus I made that change and also updated the text of the message it logs to be more accurate.

https://github.com/mykmelez/gecko-dev/commit/1be1afd
https://hg.mozilla.org/mozilla-central/rev/0c0176febdf0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Myk, I am sure that you are going to do it but don't hesitate to submit the uplift request!
Flags: needinfo?(myk)
Comment on attachment 8398638 [details] [diff] [review]
patch v2: fix review nits

(In reply to Myk Melez [:myk] [@mykmelez] from comment #7)
> I'll file a followup bug once this lands.

I filed this as bug 990274.


[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Bug 934756 (the Synthetic APKs reimplementation of the Android web runtime).

User impact if declined: 
  Users of homescreen shortcuts created by the old implementation of the
  runtime will no longer be able to use the shortcuts, which will suddenly
  stop working.

Testing completed (on m-c, etc.): 
  This patch landed on mozilla-central a few days ago and hasn't caused
  any regressions.

Risk to taking this patch (and alternatives if risky): 
  The patch is relatively low-risk, as its changes are restricted to a single
  runtime-specific file.  It does fix a couple of semi-related issues in the
  process, and I could make it a bit smaller (and thus slightly lower risk),
  but then we'd lose the benefit of landing the exact same patch we're testing
  on Nightly.  Better to land this one, I think.

String or IDL/UUID changes made by this patch:
  The patch copies one non-localizable string from the old implementation
  to the new one.  This doesn't affect localization, since the old string
  wasn't localizable, and the new one isn't either.  Followup bug 990274
  will add localization of the string, but I won't request uplift for it,
  since it isn't critical.
Attachment #8398638 - Flags: approval-mozilla-beta?
Attachment #8398638 - Flags: approval-mozilla-aurora?
Flags: needinfo?(myk)
Attachment #8398638 - Flags: approval-mozilla-beta?
Attachment #8398638 - Flags: approval-mozilla-beta+
Attachment #8398638 - Flags: approval-mozilla-aurora?
Attachment #8398638 - Flags: approval-mozilla-aurora+
Using Firefox 29 beta 6 I'm able to launch apps installed as shortcuts, before and after updating them.
Verifying as fixed on version 29 

Still needs to be verified on Aurora and Nightly
Verified as fixed in builds:
- 31.0a1 (2014-04-10);
- 30.0a2 (2014-04-10);
Device: Asus Transformer Tab (Android 4.2.1).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.