Closed Bug 1029674 Opened 5 years ago Closed 5 years ago

Marketplace packaged app shows white screen of sorrow on launch

Categories

(Firefox Graveyard :: Webapp Runtime, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 33

People

(Reporter: myk, Assigned: marco)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
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).
Attached patch WIP (obsolete) — Splinter Review
The custom origin was indeed the problem. I'll try to write a test case.
Attachment #8446652 - Flags: feedback?(myk)
Comment on attachment 8446652 [details] [diff] [review]
WIP

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

Seems reasonable to me!
Attachment #8446652 - Flags: feedback?(myk) → feedback+
Attached patch Patch (obsolete) — Splinter Review
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)
Assignee: nobody → mar.castelluccio
Attachment #8446652 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
Now works on Mac too: https://tbpl.mozilla.org/?tree=Try&rev=d93f6e44e254
Attachment #8447559 - Attachment is obsolete: true
Attachment #8447776 - Flags: review?(myk)
Attached patch PatchSplinter Review
I forgot to add the files to regenerate the signed app package.
Attachment #8447776 - Attachment is obsolete: true
Attachment #8447776 - Flags: review?(myk)
Attachment #8447786 - Flags: review?(myk)
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;
  };
Attachment #8447786 - Flags: review?(myk)
Attachment #8447786 - Flags: review?(dkeeler)
Attachment #8447786 - Flags: review+
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}
Attachment #8447786 - Flags: review?(dkeeler) → review+
https://hg.mozilla.org/mozilla-central/rev/2ec73f026a68
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Depends on: 1035282
No longer depends on: 1035282
Duplicate of this bug: 1064688
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.