619494 - e10s: Make IndexedDB work in multi-process Fennec

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Matt Brubeck (:mbrubeck)]

In current Fennec builds, the moz_indexedDB object exists, but when open() is used in content it fails to prompt for user permission, and neither the onsuccess or onerror callbacks are ever reached.

(Note: If this is not fixed before the next Fennec release, we should file a separate bug to temporarily remove the moz_indexedDB property in Fennec builds.)

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

doug, can put a plan for this in the bug please? I don't know if we can block on it or not this late in the game.

Comment 2

[Josh Matthews [:jdm]]

Dunno if anyone else has done any analysis of what's needed here, but I'll start putting some thoughts down.

The obvious problem is that we don't have a profile in content, so we can't just open up a database connection.  However, this problematic bit is contained in OpenDatabaseHelper::DoDatabaseWork (IDBFactory.cpp), which is basically a runnable that executes on the DB thread.  The only other problematic bit in IDBFactory.cpp that I can see is in the use of CheckPermissionsHelper, which attempts to change permissions.

I haven't dug through anything but opening a database so far.  I wonder if we could get away with just remoting the specific runnables somehow?

Comment 3

[Doug Turner (:dougt)]

right.  also, the prompting that the code does needs to be remoted (similar to what we did with geo).

Comment 4

[Doug Turner (:dougt)]

IndexedDB will not make Fennec 4.0.  Bug 623316 will remove this feature from the content dom.

Comment 5

[Doug Turner (:dougt)]

furthermore...  we want the database to be accessed from the content process so that we can more easily support multi-content-process firefox.

Comment 6

[Benjamin Smedberg]

I think you mean *don't want* there...

Comment 7

[Alon Zakai (:azakai)]

Attachment: wip patch

Part I: Remoting the permission prompt.

So, the situation is this: IndexedDB fires events, and a browser.js object on desktop listens for them with the observer service. That object shows the prompts, and sends a response back down, yes/no/unknown.

So my first step here was to create a corresponding object in mobile. But with a parent and a child part, with message passing between them. So the child hears the observer service message, tells the parent, which shows the prompt, which tells the child, which sends a response back down.

A did some minor refactoring of OfflineApps.js, since the prompt we show is basically identical - even down to the text shown ("X wants to store stuff on your computer"). So sharing code there sounded like it made sense.

This patch sort of works, however, mfinkle has explained to me that there are in fact two ways to show a permission prompt, and the OfflineApps.js is the old way. I assumed it called the proper stuff underneath, and that by sharing code with OfflineApps I was automatically doing the right thing, but apparently not?

Options to proceed:

* Continue sharing code with OfflineApps as in this patch
* Start again, do not share code with that, use ContentPermissionPrompt instead
* Continue to share code with OfflineApps, but rewrite it to use ContentPermissionPrompt

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 523462 [details] [diff] [review]
wip patch

Approach Feedback:
* Using the same prompt as Offline Apps is fine for now. If we decide to change, we should change both prompts (IndexDB and OfflineApps)

Code Feedback:
* Use 'let' over 'var' for local variables
* No need to wrap at 80 in front-end code
* Move IndexedDBPromptHelperParent into OfflineApps.js (maybe just move the code into OfflineApps class too)
** We moved non-critical code out of browser.js to make the file smaller
** Make sure you update the lazy load script
* Drop the *Child part of IndexedDBPromptHelperChild

>  init:
>  function IndexedDBPromptHelper_init() {

One line is fine

> function IndexedDBPromptHelper_observe(subject, topic, data) {

add "a" prefix to args

Comment 9

[Doug Turner (:dougt)]

I would like to see the use the ContentPermisionPrompt, but I may be bias.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #9)
> I would like to see the use the ContentPermisionPrompt, but I may be bias.

You are, but that's OK. Currently OfflineApps and IndexDB are very similar in concept and I think we should stick to the current OfflineApps for now. We can file a new bug to switch both over at the same time.

Comment 11

[Alon Zakai (:azakai)]

Hmm, moving on to acting on the results of the prompt, the IndexedDB code tries to set the permission in the child process, which fails. So looks like additional rewriting will be needed for this to work. I am starting to suspect that refactoring OfflineApps yet more so it supports IndexedDB permission writing may become counterproductive especially as it is deprecated. Investigating the options now.

Comment 12

[Alon Zakai (:azakai)]

To clarify the scope of this bug, following irc discussion with bsmedberg, I will do the work we need for things to work in Fennec, which is (1) set up cross-process prompts (this will help mainly Fennec as the code is not shared, we use different prompts I am afraid, but getting it in Firefox would probably be easy) - and (2) some minor platform changes, mainly to remote the location of the database files and handle the inability to change permissions in the child process, etc.

Additional work to remove filesystem access from the child process entirely can be done in a followup bug. That won't be actually needed for things to work, but would be necessary for future sandboxing of the child process.

Comment 13

[Benjamin Smedberg]

It will be needed if we have multiple content processes, or if we ever load content from that domain in the chrome process. So you at least need to assert that we only ever open that database file from the content process, and never from the chrome process.

Comment 14

[Alon Zakai (:azakai)]

Attachment: patch

Asking mfinkle to review the mobile/* stuff. Does anyone know who can review the dom/indexedDB parts?

Comment 15

[Benjamin Smedberg]

bent

Comment 16

[Alon Zakai (:azakai)]

Comment on attachment 524739 [details] [diff] [review]
patch

Thanks.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 524739 [details] [diff] [review]
patch

>diff --git a/mobile/chrome/content/IndexedDB.js b/mobile/chrome/content/IndexedDB.js

>+  showPrompt: function(aMessage) {

>+    if (topic == this._permissionsPrompt) {
>+      type = "indexedDB";
>+      payload.responseTopic = this._permissionsResponse;
>+    }
>+    else if (topic == this._quotaPrompt) {
>+      type = "indexedDBQuota";
>+      payload.responseTopic = this._quotaResponse;
>+    }
>+    else if (topic == this._quotaCancel) {
>+      payload.permission = Ci.nsIPermissionManager.UNKNOWN_ACTION;
>+      browser.messageManager.sendAsyncMessage("IndexedDB:Response", payload);
>+      // XXX Need to actually save this?
>+      return;
>+    }

nit: cuddle the else if and else

>+    let timeoutId = setTimeout(function() {
>+      payload.permission = Ci.nsIPermissionManager.UNKNOWN_ACTION;
>+      browser.messageManager.sendAsyncMessage("IndexedDB:Response", payload);
>+      timeoutId = null;
>+    }, 30000);

Add a comment above this block so we know that we are defaulting to unknown if the user delays too long

>diff --git a/mobile/chrome/content/bindings/browser.js b/mobile/chrome/content/bindings/browser.js
>+let IndexedDB = {

>+  uninit: function IndexedDBPromptHelper_uninit() {
>+    let os = Services.obs;
>+    os.removeObserver(this, this._permissionsPrompt, false);
>+    os.removeObserver(this, this._quotaPrompt, false);
>+    os.removeObserver(this, this._quotaCancel, false);
>+    removeMessageListener("IndexedDB:Response", this);
>+  },


>diff --git a/mobile/chrome/content/browser-ui.js b/mobile/chrome/content/browser-ui.js

>+    messageManager.addMessageListener("IndexedDB:Prompt", function(aMessage) {
>+      return IndexedDB.receiveMessage(aMessage);
>+    });
>+

Add a comment above this block to let us know we are using a anon function to act as a wrapper to delay load the IndexedDB.js file
and a bug number for the general fix.

>diff --git a/mobile/chrome/content/browser.js b/mobile/chrome/content/browser.js

>+Cu.import("resource://gre/modules/Services.jsm");

Do we need this? I thought we delay load Services

r+ on the /mobile parts with nits fixed

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #17)

> >diff --git a/mobile/chrome/content/browser-ui.js b/mobile/chrome/content/browser-ui.js
> 
> >+    messageManager.addMessageListener("IndexedDB:Prompt", function(aMessage) {
> >+      return IndexedDB.receiveMessage(aMessage);
> >+    });
> >+
> 
> Add a comment above this block to let us know we are using a anon function to
> act as a wrapper to delay load the IndexedDB.js file
> and a bug number for the general fix.

So we can update it when we fix bug 647079

Comment 19

[Shawn Wilsher :sdwilsh]

Can we make sure this gets sr too please?

Comment 20

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

Comment on attachment 524739 [details] [diff] [review]
patch

So bsmedberg's comment needs to be addressed... There's no code here to guard via assertions (or, preferably, forbid) the chrome process accessing indexedDB databases.

In particular, the PageInfo dialog in Firefox queries and clears IndexedDB from chrome. I don't think that functionality exists in Fennec right now, but it presumably will in the future. We need to have some sort of plan in place for this.

I guess we need to figure out how much of a priority this is for the near future. If we decide to do this for right now we need some hard assertions to ensure that we don't have races. If we decide to wait until proper remoting can be done then we should just skip this.

>+++ b/dom/indexedDB/IDBFactory.cpp
> ...
>+#include "mozilla/dom/ContentChild.h"
>+
> #include "IDBFactory.h"

Please don't include this before IDBFactory.h to make sure that the IDBFactory.h header compiles on its own.

>+nsString IDBFactory::mDBDirectory;

Static nsStrings are a no-no (at best they screw up leak statistics, at worst they can cause crashes). You either need to do the call each time in IDBFactory::Open or find some other way to manage the lifetime of that string better.

>+ContentParent::RecvGetIndexedDBDirectory(nsString* aDirectory)

This isn't returning "the IndexedDB directory"... It's returning the profile directory. I'd rename.

Comment 21

[Alon Zakai (:azakai)]

(In reply to comment #20)
> Comment on attachment 524739 [details] [diff] [review]
> patch
> 
> So bsmedberg's comment needs to be addressed... There's no code here to guard
> via assertions (or, preferably, forbid) the chrome process accessing indexedDB
> databases.

Well, the thing is, Firefox does access databases in the chrome process, while Fennec doesn't. There is no simple way to detect whether a build is Firefox or Fennec (by design), so for such checks we would need to add something more involved, like sending a message to say "this DB was accessed in this process - do not allow it to be accessed in yours".

It's a little tricky to get this right (timing), and this code will become unnecessary when Firefox also has remote tabs. So this seems fairly complex and temporary, for little benefit, and I am in favor of not doing it. Unless there is a better way?

> 
> In particular, the PageInfo dialog in Firefox queries and clears IndexedDB from
> chrome. I don't think that functionality exists in Fennec right now, but it
> presumably will in the future. We need to have some sort of plan in place for
> this.

I assumed we wanted to wait for that for proper indexedDB remoting later on - am I wrong?

Comment 22

[Alon Zakai (:azakai)]

Attachment: patch v2

Patch fixes all issues raised except for my questions in the last comment, and the matter of ContentChild.h being included at the beginning of IDBFactory.cpp.

The problem with that is that we must include basictypes.h before prtypes.h (basictypes is from chromium ipc code, prtypes from nspr). ptypes is included early in almost everything, and ContentChild needs basictypes, so including ContentChild later down will not work - it has to be first. Unless we want to do ContentChild.h later down, but add basictypes.h at the top of the file. That's even more confusing in my opinion, and in any case IDBFactory.h cannot be on top.

Comment 23

[Alon Zakai (:azakai)]

Attachment: patch v3

Updated patch following IRC discussion about the remaining issues.

Comment 24

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

Comment on attachment 527686 [details] [diff] [review]
patch v3

Thanks Alon, this looks much better.

Can you please move all the caching stuff (the helper class, the observer service use, the static nsString) to ContentProcessChild? It really has nothing to do with IndexedDB and shouldn't live there.

Also, anything that can fail should not happen inside IDBFactory's constructor. Let's move the 'GetDirectory' call to IDBFactory::Open so that an error can be properly handled.

Please rename IDBFactory::GetDirectory to GetIndexedDBDirectory to be less vague.

Please ping me on IRC if you have any questions?

Comment 25

[Alon Zakai (:azakai)]

Attachment: patch v4

Moved the caching into ContentChild, which solves all the comments except for renaming GetDirectory.

I called it GetDirectory to make its place clear in respect to the already existing GetDirectoryForOrigin. I suggest that we either rename them both, or neither.

Comment 26

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

Comment on attachment 528490 [details] [diff] [review]
patch v4

Review of attachment 528490 [details] [diff] [review]:

r=me with these things fixed.

::: dom/indexedDB/IDBFactory.cpp
@@ +630,5 @@
 // static
+void
+IDBFactory::NoteUsedByProcessType(GeckoProcessType aProcessType)
+{
+  if (gAllowedProcessType != GeckoProcessType_Invalid) {

I prefer == check and reversing the blocks since both conditions have a block.

@@ +650,5 @@
+    NS_ENSURE_SUCCESS(rv, rv);
+    rv = (*aDirectory)->Append(NS_LITERAL_STRING("indexedDB"));
+    NS_ENSURE_SUCCESS(rv, rv);
+  } else {
+    nsCOMPtr<nsILocalFile> localDirectory = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID);

Nit: Looks like this breaks 80 chars, please break into two lines.

@@ +651,5 @@
+    rv = (*aDirectory)->Append(NS_LITERAL_STRING("indexedDB"));
+    NS_ENSURE_SUCCESS(rv, rv);
+  } else {
+    nsCOMPtr<nsILocalFile> localDirectory = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID);
+    rv = localDirectory->InitWithPath(ContentChild::GetSingleton()->GetIndexedDBPath());

Here too.

::: dom/ipc/ContentChild.cpp
@@ +231,5 @@
 
 ContentChild::~ContentChild()
 {
+    if (gIndexedDBPath)
+        delete gIndexedDBPath;

delete is null-safe, so don't check for gIndexedDBPath.

::: dom/ipc/ContentParent.cpp
@@ +450,5 @@
+    nsCOMPtr<nsIFile> dbDirectory;
+    nsresult rv = indexedDB::IDBFactory::GetDirectory(getter_AddRefs(dbDirectory));
+
+    if (NS_FAILED(rv))
+        return true;

I think you need to issue an NS_ERROR here... That should never really fail, and if it does we need to know about it.

Comment 27

[Alon Zakai (:azakai)]

http://hg.mozilla.org/mozilla-central/rev/9e358195a677

Comment 28

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Is there a followup yet for remoting IDB?  Can you please set it to block bug 641683 and CC me and bent?  We discussed what's needed for that a bit at the last all-hands.

Comment 29

[Cédric Corazza]

Alon,
for http://hg.mozilla.org/mozilla-central/rev/9e358195a677 , shouldn't computer be replaced with *mobile* for offlineApps.available and indexedDBQuota.siteWantsTo entities?

Comment 30

[Alon Zakai (:azakai)]

Very good point!

Asking for UI guidance.

Comment 31

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #30)
> Very good point!
> 
> Asking for UI guidance.

I added a note in the "rewrite strings for mobile" bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=582048#c3