1029674 - Marketplace packaged app shows white screen of sorrow on launch

Status: RESOLVED FIXED

(Firefox Graveyard :: Webapp Runtime, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

There are two Marketplace packaged apps in Marketplace:

https://marketplace.firefox.com/app/marketplace-package
https://marketplace.firefox.com/app/marketplace

And both can be installed and used on Android (and presumably FxOS).  But when I install and launch them on Mac, they both display a white screen of sorrow.

Console output is unilluminative:

06-24 08:56 > /Applications/Marketplace.app/Contents/MacOS/webapprt 
2014-06-24 10:59:45.078 webapprt[73373:507] MY WEBAPPRT BUILDID: 20140623030201
2014-06-24 10:59:45.079 webapprt[73373:507] found override firefox binary: org.mozilla.nightly
2014-06-24 10:59:45.081 webapprt[73373:507] USING FIREFOX : /Applications/FirefoxNightly.app
2014-06-24 10:59:45.082 webapprt[73373:507] Looking for firefox ini file here: /Applications/FirefoxNightly.app/Contents/MacOS/application.ini
2014-06-24 10:59:45.082 webapprt[73373:507] FIREFOX WEBAPPRT BUILDID: 20140623030201
2014-06-24 10:59:45.082 webapprt[73373:507] This Application has the newest webrt.  Launching!
2014-06-24 10:59:45.083 webapprt[73373:507] Set XUL_APP_FILE to: /Applications/Marketplace.app/Contents/MacOS/webapp.ini
2014-06-24 10:59:45.098 webapprt[73373:507] WebappRT application.ini path: /Applications/FirefoxNightly.app/Contents/MacOS/webapprt/webapprt.ini
2014-06-24 10:59:45.099 webapprt[73373:507] setting app profile: marketplace-48ae30af1a6f84a65338f3e1a91d8a67
!! Creating a dummy channel for {7b914b11-e6f1-a840-ba42-3c525f6859ec} (no appInfo)


But, strangely, the app works on a debug build (after I set FirefoxBinary to org.mozilla.nightlydebug in its Info.plist file), although it displays the following error message in the console:

JavaScript error: chrome://webapprt/content/webapp.js, line 127: WebappRT.config is undefined

And if I then launch the app with my regular nightly build, it continues to work.

Comment 1

[Marco Castelluccio [:marco]]

I haven't investigated further, but looks like we're trying to get the app info using the ID "{7b914b11-e6f1-a840-ba42-3c525f6859ec}" (or another generated ID), while the app in the registry is stored with the ID "marketplace.firefox.com".

It's almost surely due to the ID changing code (probably we're using the old ID while installing the app).

Comment 2

[Marco Castelluccio [:marco]]

Attachment: WIP

The custom origin was indeed the problem. I'll try to write a test case.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8446652 [details] [diff] [review]
WIP

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

Seems reasonable to me!

Comment 4

[Marco Castelluccio [:marco]]

Attachment: Patch

Try runs were quite slow today.

This is a try run (that was failing on Mac because I forgot to rename a function...):
https://tbpl.mozilla.org/?tree=Try&rev=4839f99f9fab

This is a Mac-only try run with the patch fixed:
https://tbpl.mozilla.org/?tree=Try&rev=d95ee4e2b324

(The Mac run hasn't finished yet, I'll see the results tomorrow)

Comment 5

[Marco Castelluccio [:marco]]

Attachment: Patch

Now works on Mac too: https://tbpl.mozilla.org/?tree=Try&rev=d93f6e44e254

Comment 6

[Marco Castelluccio [:marco]]

Attachment: Patch

I forgot to add the files to regenerate the signed app package.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8447786 [details] [diff] [review]
Patch

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

A Security - Mozilla PSM Glue peer will need to review the security/manager/ changes, so requesting review from dkeeler.

::: toolkit/webapps/MacNativeApp.js
@@ +33,5 @@
> + * apps. In production code, it's "/Applications". In tests, it's
> + * "~/Applications" because on build machines we don't have enough privileges
> + * to write to the global "/Applications" directory.
> + */
> +NativeApp._rootInstallDir = LOCAL_APP_DIR;

This is ok, especially since individual apps never override the value, but I wonder why it's necessary, since the tests could already override the value for all apps simply by modifying NativeApp.prototype._rootInstallDir.

::: toolkit/webapps/tests/head.js
@@ +405,5 @@
> +                              getInterface(Ci.nsIWebNavigation).
> +                              QueryInterface(Ci.nsIDocShell).
> +                              chromeEventHandler.ownerDocument.defaultView.
> +                              PopupNotifications.panel;
> +  

Nit: this line has trailing whitespace.

06-30 09:55 > git apply ~/fix_installation_of_apps_with_custom_origin
/Users/myk/fix_installation_of_apps_with_custom_origin:1382: trailing whitespace.
  
warning: 1 line adds whitespace errors.

@@ +409,5 @@
> +  
> +  popupPanel.addEventListener("popupshown", function onPopupShown() {
> +    popupPanel.removeEventListener("popupshown", onPopupShown, false);
> +    this.childNodes[0].button.doCommand();
> +  }, false);

See attachment 8445433 [details] [diff] [review] in bug 1000315 for a fix to this implementation of confirmNextInstall!  But also see bug 1000315, comment 46 for my question about the fix.

::: toolkit/webapps/tests/test_custom_origin.xul
@@ +60,2 @@
>  
> +  TrustedRootCertificate.index = Ci.nsIX509CertDB.AppXPCShellRoot;

Nit: elsewhere, you change the value of a property before registering a cleanup function that changes it back, and that seems like a more natural order.

@@ +60,5 @@
>  
> +  TrustedRootCertificate.index = Ci.nsIX509CertDB.AppXPCShellRoot;
> +
> +  // Allow signed apps to be installed from the test origin
> +  let old_signed_app_origins;

Nit: I'd still use camelCase names for this variable (and new_signed_app_origins), even though they represent a pref with an underscore_separated_name.

@@ +80,5 @@
>    }
>  
> +  confirmNextInstall();
> +
> +  let deferredInstall = Promise.defer()

Nit: trailing semicolon.

@@ +86,5 @@
> +  request.onerror = function() {
> +    deferredInstall.reject(this.error.name);
> +  };
> +  request.onsuccess = deferredInstall.resolve;
> +  yield deferredInstall.promise;

Nice use of a deferred to wrap a callback-based API!

@@ +91,5 @@
> +
> +  let appObject = request.result;
> +  ok(appObject, "app is non-null");
> +
> +  let deferredDownload = Promise.defer();

Nit: it might be worth noting here that the download happens automatically, as it was surprising at first to see download handlers being attached without a download being initiated.

@@ +96,5 @@
> +  appObject.ondownloaderror = function() {
> +    deferredDownload.reject(this.error.name);
> +  };
> +  appObject.ondownloadapplied = deferredDownload.resolve;
> +  yield deferredDownload.promise;

You could avoid having to come up with unique names for the deferred objects by using a let statement to scope them to the code that uses them:

  let (deferred = Promise.defer()) {
    appObject.ondownloaderror = function() {
      deferred.reject(this.error.name);
    };
    appObject.ondownloadapplied = deferred.resolve;
    yield deferred.promise;
  };

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8447786 [details] [diff] [review]
Patch

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

The changes to security/manager/... look good to me.

::: security/manager/ssl/tests/unit/test_signed_apps/gentestfiles/create_test_files.sh
@@ +169,5 @@
>  # to avoid that reverting again
>  PRIV_UID=`uuidgen`
>  signApp $DB_PATH ${BASE_PATH}/unsigned_app_origin.zip \
>          $TEST_APP_PATH/origin_app_1.zip \
> +        $PRIV_UID 1 ${TRUSTED_EE}

Oh dear. I guess this was always wrong?

@@ +202,2 @@
>          echo cp ${TEST_APP_PATH}/*  ${TEST_APP_PATH}/../unsigned_app_1.zip ${BASE_PATH}/..
> +        echo cp ${TEST_APP_PATH}/*  ${BASE_PATH}/${TOOLKIT_WEBAPPS_TEST_LOC}

nit: I'm not sure why there are two spaces before ${TEST_APP_PATH}, etc., but only one is necessary

@@ +207,5 @@
>  done
>  
>  cp ${TEST_APP_PATH}/* ${BASE_PATH}/${APPS_TEST_LOC}
>  cp ${TEST_APP_PATH}/* ${TEST_APP_PATH}/../unsigned_app_1.zip ${BASE_PATH}/..
> +cp ${TEST_APP_PATH}/*  ${BASE_PATH}/${TOOLKIT_WEBAPPS_TEST_LOC}

nit: only one space is necessary before ${BASE_PATH}

Comment 9

[Marco Castelluccio [:marco]]

https://tbpl.mozilla.org/?tree=Try&rev=4a1d96139ba0

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ec73f026a68

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/2ec73f026a68