827346 - indexedDB is deactivated in WebAppRT

Status: VERIFIED FIXED

(Firefox Graveyard :: Webapp Runtime, defect)

Description

[Kohei Yoshino]

Attachment: 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.

Comment 1

[Kohei Yoshino]

Testing with Aurora for Mac.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Is this with the desktop Firefox or with B2G-Desktop? Any chance you can post your html/js that uses indexedDB?

Comment 3

[Kohei Yoshino]

On my desktop Firefox. Here's the app: http://www.bzdeck.com/ The code is not minified for easier browsing.

Comment 4

[Kohei Yoshino]

To be clear: it works well on Aurora, but after installed as an Open Web App, indexedDB.open() fails.

Comment 5

[Kohei Yoshino]

Attachment: Error Console screenshot

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

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I'm a bit bogged down in B2G stuff at the moment but I'll try to investigate soon.

Comment 7

[mathieu]

Attachment: test 1/2

Comment 8

[mathieu]

Attachment: test 2/2

Comment 9

[Abhishek Potnis [:avp]]

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.

Comment 10

[Kohei Yoshino]

abhishekp: thanks for your confirmation! So it can be an issue in any app which implements IndexedDB.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Facing the same problem on MacOS X.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Furthermore, in my case, the |readyState| of the |open()| operation remains pending forever. No error message on any console.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Attachment: Activating indexedDB in WebAppRT

Untested yet, but that might be sufficient to fix the bug.

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

If the pref were false window.indexedDB wouldn't even exist.

Comment 15

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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?

Comment 17

[David Teller [:Yoric] - still alive but not very active]

I haven't found a way to run a webapp in a self-compiled webapprt. How would I do that?

Comment 18

[Marco Castelluccio [:marco]]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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);

Comment 19

[Marco Castelluccio [:marco]]

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.

Comment 20

[Marco Castelluccio [:marco]]

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)

Comment 21

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

(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.

Comment 22

[Marco Castelluccio [:marco]]

Attachment: Patch

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.

Comment 23

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

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?

Comment 24

[Marco Castelluccio [:marco]]

Attachment: Patch

(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).

Comment 25

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

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();
  }

Comment 26

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

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.

Comment 27

[Drew Willcoxon :adw]

When did they stop working?  I haven't been following WebAppRT for a while, so I'm not sure how helpful I can be.

Comment 28

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

(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.

Comment 29

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 30

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

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.

Comment 31

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

Setting checkin-needed while waiting for Inbound to reopen.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

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

Comment 33

[Ryan VanderMeulen [:RyanVM]]

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

Comment 34

[Kohei Yoshino]

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