Closed Bug 922187 Opened 6 years ago Closed 5 years ago

mozApps.checkInstalled out of memory

Categories

(Core Graveyard :: DOM: Apps, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla33

People

(Reporter: sole, Assigned: marco)

Details

Attachments

(1 file, 1 obsolete file)

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!
Component: Webapp Runtime → DOM: Apps
Product: Firefox → Core
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')
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
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.
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
Attachment #8440170 - Flags: review?(myk)
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.
Attachment #8440170 - Flags: review?(myk) → review+
Do you have a better suggestion for the error name? INVALID_URL may be misleading. Maybe "CANT_LOAD_URL"?
Flags: needinfo?(myk)
Another (probably better) idea would be "URL_NOT_ALLOWED".
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.
Flags: needinfo?(myk)
Attached patch PatchSplinter Review
There's a test relying on the old behavior that was failing.
Attachment #8440170 - Attachment is obsolete: true
Attachment #8444177 - Flags: review?(myk)
OS: Mac OS X → All
Hardware: x86 → All
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).
Attachment #8444177 - Flags: review?(myk) → review+
Oh, I updated the error name in the code but not in the test...
Flags: needinfo?(mar.castelluccio)
https://hg.mozilla.org/mozilla-central/rev/bbb69edbebf1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.