922187 - mozApps.checkInstalled out of memory

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Soledad Penades [:sole] [:spenades]]

When attempting to determine if an app is installed with mozApps.checkInstalled, using a manifest in another domain results in a NS_ERROR_DOM_BAD_URI: Access to restricted URI denied @ resource://gre/components/Webapps.js:170 error in Firefox 24 and Firefox 25 Beta.

Using Aurora and Nightly 27, I get an "out of memory" exception.

Test case here:
http://people.mozilla.org/~spenades/test_cases/checkinstalled/

where I'm attempting to check if the app is installed but using a different domain:port (localhost:8888) instead of people.mozilla.org


On a side note, once the domain of the site and the manifest match, Firefox 24 and 25 falsely report the app as installed but it is not. Aurora and Nightly correctly report the app as not installed. Not sure if it's worth filing that bug as it seems to have been corrected already!

Comment 1

[Bill Walker [:bwalker] [@wfwalker]]

I just reproduced this on Mac OS X 32.0a1 and 29.0.1 by typing the following into the command-line:

window.navigator.mozApps.checkInstalled('http://youzeek.com/youzeek.webapp')

Comment 2

[Bill Walker [:bwalker] [@wfwalker]]

When I run with FirefoxNightlyDebug build, I see the following in console output:

System JS : ERROR resource://gre/components/Webapps.js:184 - NS_ERROR_DOM_BAD_URI: Access to restricted URI denied

Comment 3

[Marco Castelluccio [:marco]]

I think the "NS_ERROR_DOM_BAD_URI" is expected because checkInstall only accepts manifests on the same domain of the page.

The "out of memory" may probably be a bogus error due to us not catching exceptions thrown by http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#184, the real error is still the security one.

Indeed calling checkInstalled in the browser console, that is a privileged environment, doesn't fail.

Comment 4

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 5

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

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

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

::: dom/apps/src/Webapps.js
@@ +195,5 @@
> +          oid: this._id,
> +          requestID: this.getRequestId(request) });
> +    } catch (ex) {
> +      Services.DOMRequest.fireErrorAsync(request, "INVALID_URL");
> +    }

Nit: I'd surround only the checkMayLoad call with the try/catch block, as the exception it may throw is the only one we expect to catch here.

Comment 6

[Marco Castelluccio [:marco]]

Do you have a better suggestion for the error name? INVALID_URL may be misleading. Maybe "CANT_LOAD_URL"?

Comment 7

[Marco Castelluccio [:marco]]

Another (probably better) idea would be "URL_NOT_ALLOWED".

Comment 8

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

checkMayLoad throws NS_ERROR_DOM_BAD_URI but reports CheckSameOriginError, which is "Security Error: Content at %S may not load data from %S."  DOMApplicationRegistry meanwhile sometimes complains INSTALL_FROM_DENIED when an origin tries to install an app it isn't allowed to install, but that doesn't do a particularly good job of explaining why the installation request was denied.

So I would make this something like CROSS_ORIGIN_URL_NOT_ALLOWED to make it more obvious that the reason it isn't allowed is that the origin of the URL is different from the origin of the requesting page.

Comment 9

[Marco Castelluccio [:marco]]

Attachment: Patch

There's a test relying on the old behavior that was failing.

Comment 10

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

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

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

Nit: on second thought, CROSS_ORIGIN_CHECK_NOT_ALLOWED is probably a better name for this error.  After all, it's the *check* that is cross-origin (and that isn't allowed).

Comment 11

[Marco Castelluccio [:marco]]

Pushed with nit fixed!

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5e0bb1653b5

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/9324051fd22c for a failure in mochitest-4: https://tbpl.mozilla.org/php/getParsedLog.php?id=42305541&tree=Mozilla-Inbound

Comment 13

[Marco Castelluccio [:marco]]

Oh, I updated the error name in the code but not in the test...

Comment 14

[Marco Castelluccio [:marco]]

Fixed and relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/bbb69edbebf1

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bbb69edbebf1