Closed Bug 827346 Opened 7 years ago Closed 7 years ago

indexedDB is deactivated in WebAppRT

Categories

(Firefox Graveyard :: Webapp Runtime, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 25

People

(Reporter: kohei, Assigned: marco)

References

Details

Attachments

(5 files, 3 obsolete files)

Attached file App manifest
I've tried to convert my app to an Open Web App (OWA/mozApp), but opening an IndexedDB database silently failed without no errors. Testing with Aurora.

> try {
>   let req = indexedDB.open('MyApp');
>   alert(req);
>   req.addEventListener('error', function (event) {
>     alert(event);
>   });
>   req.addEventListener('upgradeneeded', function (event) {
>     alert(event);
>   });
>   req.addEventListener('success', function (event) {
>     alert(event);
>   });
> } catch (ex) {
>   alert(ex);
> }

The first alert() says [object IDBOpenDBRequest] but no other alert raises. Something odd. Attached the app manifest file.
Testing with Aurora for Mac.
Attachment #698690 - Attachment mime type: application/json → text/plain
Is this with the desktop Firefox or with B2G-Desktop? Any chance you can post your html/js that uses indexedDB?
On my desktop Firefox. Here's the app: http://www.bzdeck.com/ The code is not minified for easier browsing.
To be clear: it works well on Aurora, but after installed as an Open Web App, indexedDB.open() fails.
Here's a screenshot of the Error Console when the app is launched with -jsconsole and javascript.options.strict = true. Unsurprisingly, no errors on IndexedDB found.
https://developer.mozilla.org/en-US/docs/Apps/Tutorials/General/Debugging_the_app
I'm a bit bogged down in B2G stuff at the moment but I'll try to investigate soon.
Attached file test 1/2
Attached file test 2/2
Attachment #714365 - Attachment mime type: application/octet-stream → application/x-web-app-manifest+json
Attachment #714364 - Attachment mime type: text/plain → text/html
I am also facing the same problem. My OpenWebApp works fine with indexedDB when accessed from the browser and also on FirefoxOS Simulator, but fails when accessed from the app installed on Windows.
abhishekp: thanks for your confirmation! So it can be an issue in any app which implements IndexedDB.
Summary: indexedDB.open() silently fails in an Open Web App (OWA/mozApp) → indexedDB.open() silently fails in an Open Web App (OWA/mozApp) on desktop
Facing the same problem on MacOS X.
Furthermore, in my case, the |readyState| of the |open()| operation remains pending forever. No error message on any console.
Summary: indexedDB.open() silently fails in an Open Web App (OWA/mozApp) on desktop → Using desktop WebAppRT, indexedDB.open() returns a request that remains pending forever
Component: DOM: IndexedDB → Webapp Runtime
Product: Core → Firefox
Summary: Using desktop WebAppRT, indexedDB.open() returns a request that remains pending forever → indexedDB is deactivated in WebAppRT
Attached patch Activating indexedDB in WebAppRT (obsolete) — Splinter Review
Untested yet, but that might be sufficient to fix the bug.
If the pref were false window.indexedDB wouldn't even exist.
Good to know, thanks.
My intuition on this bug is that the WebAppRT is waiting for the doorhanger dialog confirmation that the application can indeed store data. As no such doorhanger dialog is displayed, it waits forever. Given that b2g doesn't display the doorhanger for installed applications, I figure 1/ the doorhanger is not necessary 2/ there is a simple way to bypass it.
Well the manifest has the storage permission which is supposed to skip the doorhanger and whatnot.  Is the "webapp runtime" not translating the manifest permission into the appropriate entries in the permissions manager?  What happens if you step through http://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/CheckPermissionsHelper.cpp#99?
I haven't found a way to run a webapp in a self-compiled webapprt. How would I do that?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #16)
> Well the manifest has the storage permission which is supposed to skip the
> doorhanger and whatnot.  Is the "webapp runtime" not translating the
> manifest permission into the appropriate entries in the permissions manager?

On first run, the webrt is doing:
> Services.perms.add(uri, "indexedDB", Ci.nsIPermissionManager.ALLOW_ACTION);
> Services.perms.add(uri, "indexedDB-unlimited", Ci.nsIPermissionManager.ALLOW_ACTION);
Looks like that's not the right way to add permissions to the permissions manager.

If I remove those two lines and allow any request from webapprt/ContentPermission.js, IndexedDB works.
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
The problem is that we need the application id before adding the permissions. And we don't have it because the webapps registry isn't loaded yet.

I can see two solutions:
1) Wait the webapps registry loading (this could slow down the app startup)
2) Store the local application id in one of the configuration files (in the app installation directory)
(In reply to Marco Castelluccio [:marco] from comment #20)
> The problem is that we need the application id before adding the
> permissions. And we don't have it because the webapps registry isn't loaded
> yet.
> 
> I can see two solutions:
> 1) Wait the webapps registry loading (this could slow down the app startup)
> 2) Store the local application id in one of the configuration files (in the
> app installation directory)

Waiting for the registry to load will only slow down startup on firstrun, and the slowdown is unlikely to be significant, so it's reasonable.  It's also what both the Android and the FxOS runtimes do, per:

Android: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/WebAppRT.js#53
FxOS: http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#485

So that seems like the right thing to do in the desktop runtime too.
Attached patch Patch (obsolete) — Splinter Review
Per the conversation here https://groups.google.com/forum/#!topic/mozilla.dev.gaia/Wky0ZyXWs60, if an application wants more than 5 MB it needs the "storage" permission in its manifest (I've set the limit to 50 MB here, but perhaps we don't want this limit at all on desktop?).

The offline cache permission shoudln't be needed with these prefs.

The pin-app permission should be granted only if the "storage" permission is in the app manifest.

The code in Startup.jsm will be soon needed for packaged apps permissions, this is why I didn't remove it.
Attachment #774423 - Flags: feedback?(myk)
Comment on attachment 774423 [details] [diff] [review]
Patch

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

::: dom/apps/src/Webapps.js
@@ +83,5 @@
>          res = uri.spec;
>        }
>      } catch(e) {
>        throw new Components.Exception(
> +        "INVALID_URL: '" + aURL + "'", Cr.NS_ERROR_FAILURE

Nice catch!

::: webapprt/Startup.jsm
@@ +22,5 @@
>  Cu.import("resource://webapprt/modules/WebappsHandler.jsm");
>  WebappsHandler.init();
>  
>  // On firstrun, set permissions to their default values.
>  if (!Services.prefs.getBoolPref("webapprt.firstrun")) {

Let's include a placeholder comment that explains why there aren't actually any permissions being set here, something like:

// Once we support packaged apps, set their permissions here on firstrun.

::: webapprt/prefs.js
@@ +33,5 @@
>  
> +// IndexedDB
> +pref("dom.indexedDB.enabled", true);
> +pref("indexedDB.feature.enabled", true);
> +pref("dom.indexedDB.warningQuota", 50);

What happens when an app approaches or exceeds this quota?  Does the runtime interface prompt the user for permission?
Attachment #774423 - Flags: feedback?(myk) → feedback+
Attached patch Patch (obsolete) — Splinter Review
(In reply to Myk Melez [:myk] [@mykmelez] from comment #23)
> Let's include a placeholder comment that explains why there aren't actually
> any permissions being set here, something like:
> 
> // Once we support packaged apps, set their permissions here on firstrun.
> 

Done! (also if probably the code will be modified and Startup.jsm won't be needed anymore, we'll see in bug 892837)

> What happens when an app approaches or exceeds this quota?  Does the runtime
> interface prompt the user for permission?

I'll take care of all the permissions in bug 892837 (I still have to figure out what happens on b2g and Android).
Attachment #771360 - Attachment is obsolete: true
Attachment #774423 - Attachment is obsolete: true
Attachment #775045 - Flags: review?(myk)
Comment on attachment 775045 [details] [diff] [review]
Patch

> // On firstrun, set permissions to their default values.
>+// Once we support packaged apps, set their permissions here on firstrun.
> if (!Services.prefs.getBoolPref("webapprt.firstrun")) {

Nit: these two comments don't quite make sense together.  We should either remove the first comment, leaving only the second:

  // Once we support packaged apps, set their permissions here on firstrun.
  if (!Services.prefs.getBoolPref("webapprt.firstrun")) {

Or put the second one inside the conditional block:

  // On firstrun, set permissions to their default values.
  if (!Services.prefs.getBoolPref("webapprt.firstrun")) {
    // Once we support packaged apps, set their permissions here on firstrun.


(In reply to Marco Castelluccio [:marco] from comment #24)
> Done! (also if probably the code will be modified and Startup.jsm won't be
> needed anymore, we'll see in bug 892837)

Indeed, this code will probably change regardless, since we'll need to set permissions on app update and runtime update in addition to firstrun.


> > What happens when an app approaches or exceeds this quota?  Does the runtime
> > interface prompt the user for permission?
> 
> I'll take care of all the permissions in bug 892837 (I still have to figure
> out what happens on b2g and Android).

Ok, sounds good.


One more thing: add a test that verifies that IndexedDB is now available to apps.  I haven't tested this yet, but a test named webapprt/test/content/webapprt_indexeddb.html with the following script should work (see webapprt_sample.html for the details):

  SimpleTest.waitForExplicitFinish();
  var request = indexedDB.open('test');
  request.onsuccess = function() {
    ok(true, "onsuccess should be called");
    SimpleTest.finish();
  }
Attachment #775045 - Flags: review?(myk) → review-
Once added, the test should run on:

make -C obj-dir webapprt-test-content

But that hangs for me, as does webapprt-test-chrome, both with and without this change.  So the WebappRT tests seem to be broken, at least on my Mac.  cc:ing adw for his insight into the bustage.
When did they stop working?  I haven't been following WebAppRT for a while, so I'm not sure how helpful I can be.
(In reply to Drew Willcoxon :adw from comment #27)
> When did they stop working?  I haven't been following WebAppRT for a while,
> so I'm not sure how helpful I can be.

I haven't run these tests in a while, so I'm not sure when they broke. :-(

Marco: given that the test harness doesn't seem to work, it isn't necessary to add a test; but it'd be helpful to do so anyway, so there's a test available once we get the harness back in order.

I filed bug 894162 on the bustage.
Attached patch PatchSplinter Review
Attachment #775045 - Attachment is obsolete: true
Attachment #776689 - Flags: review?(myk)
Comment on attachment 776689 [details] [diff] [review]
Patch

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

Looks good, r=myk!

::: webapprt/test/content/webapprt_indexeddb.html
@@ +3,5 @@
> +<!--
> +  This is a IndexedDB WebappRT content mochitest.  Since its name is prefixed with
> +  webapprt_, this file is picked up by the Mochitest harness.  It's just a plain
> +  mochitest that runs in the app browser within an app window.
> +-->

Nit: this isn't actually necessary in this test.  It's only in the sample test to help explain how the test harness works.
Attachment #776689 - Flags: review?(myk) → review+
Setting checkin-needed while waiting for Inbound to reopen.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f91d3bfa7706
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
I just tested my BzDeck app with the latest Nightly and confirmed IndexedDB worked as expected. Thank you everyone, great job!

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20130720 Firefox/25.0
Status: RESOLVED → VERIFIED
Duplicate of this bug: 857525
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.