Closed Bug 1149420 Opened 5 years ago Closed 5 years ago

IndexedDB permission prompt does not work in e10s tabs.


(Core :: DOM: IndexedDB, defect)

40 Branch
Windows 8.1
Not set



Tracking Status
e10s m6+ ---
firefox39 --- fixed
firefox40 --- fixed


(Reporter: jujjyl, Assigned: mrbkap)




(1 file, 2 obsolete files)


1. Visit
2. Open the page console.

Observed: the console prints "openIndexedDB: opening a IDB connection with the PERSISTENT storage option.." and nothing else.

Expected: The page should open up a query prompt "This website ( is asking to store data on your computer for offline use.", after which clicking on Allow should print in the page console:

"openIndexedDB: opening a IDB connection with the PERSISTENT storage option.."
"IDB successfully created."
"IDB successfully opened."
"No data in DB."
"Read transaction completed."

This works if running in a non-e10s window.
tracking-e10s: --- → ?
Version: Trunk → 40 Branch
Assignee: nobody → mrbkap
Attached patch patch v1 (obsolete) — Splinter Review
This turned out to be fairly straightforward, even if the setup is really awkward. I'll file a bug to clean this up a bit after the fact (in particular, if we fired an event or something at the browser that we want, we wouldn't have to worry about catching the notification in the wrong window).

The idea of this patch is that we can't use windows in the parent process (we only have one chrome window per physical browser window) so we pass the browser instead. In e10s, that's the TabParent's frame element. In Firefox (non-e10s), we can use the window's chrome event handler (I've elided the docshell part in this patch, I don't know if that's a valid thing to do in all cases, though).

I still have to fix Android.
Attachment #8590574 - Flags: review?(bent.mozilla)
Attached patch Possible patch for Android. (obsolete) — Splinter Review
Attachment #8590575 - Flags: review?(mark.finkle)
Mark, I haven't tested my Android patch at all, I wrote it based solely on what I thought I understood from reading Android code (in particular, I believe that the chrome event handler on Android is still a <xul:browser>, that there's only one list of such browsers, and that getTabForBrowser is the proper replacement). Let me know if I'm mistaken in any of my assumptions.
Comment on attachment 8590575 [details] [diff] [review]
Possible patch for Android.

Your assumptions look right. Based on what I see you doing in the Desktop patch, this should work as expected.
Attachment #8590575 - Flags: review?(mark.finkle) → review+
Comment on attachment 8590574 [details] [diff] [review]
patch v1

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

Looks good!

::: dom/indexedDB/ActorsChild.cpp
@@ +1130,5 @@
>      nsCOMPtr<nsPIDOMWindow> window = mFactory->GetParentObject();
>      MOZ_ASSERT(window);
> +    nsCOMPtr<Element> ownerElement =
> +        do_QueryInterface(window->GetChromeEventHandler());

This should be MOZ_ASSERT-able right? Otherwise we need some kind of error path.

Nit: 2 space indent

::: dom/indexedDB/PermissionRequestBase.cpp
@@ +11,5 @@
>  #include "nsIObserverService.h"
>  #include "nsIPrincipal.h"
>  #include "nsPIDOMWindow.h"
>  #include "nsXULAppAPI.h"
> +#include "mozilla/dom/Element.h"

Nit: alphabetize, before the 'ns' headers please.

::: dom/ipc/TabParent.cpp
@@ +1120,5 @@
>    } else {
>      MOZ_CRASH("Figure out security checks for bridged content!");
>    }
> +  if (!mFrameElement) {

Nit: Might as well make this NS_WARN_IF()? (Not sure why I didn't do that previously)
Attachment #8590574 - Flags: review?(bent.mozilla) → review+
Blake, you should be able to enable the prompt tests for e10s now, remove "e10s" from I think? (You might have to disable some of the others individually)
(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #6)
> Blake, you should be able to enable the prompt tests for e10s now

I spent today looking at this and it was much harder than expected. The problem is that the tests interact both with chrome and content pretty extensively and have to be altered to deal with multiple processes. There are helpers to do this, but I ended up running into problems getting the drop-down to show up for the "deny" tests. I'll file a followup to do this.
I combined the two patches into one. I'm pushing this to try now.
Attachment #8590574 - Attachment is obsolete: true
Attachment #8590575 - Attachment is obsolete: true
Attachment #8591168 - Flags: review+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment on attachment 8591168 [details] [diff] [review]
With comments addressed

This is a dependence for bug 1151495.
Attachment #8591168 - Flags: approval-mozilla-aurora?
Comment on attachment 8591168 [details] [diff] [review]
With comments addressed

Approved for uplift to beta.  Looks like we need this in 39 (beta) so that bug 1151495 can land without breaking tests.   

This is already on 40, so no longer needs uplift to aurora.
Attachment #8591168 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
QA Whiteboard: [good first verify][verify in Nightly only]
QA Whiteboard [bugday-20150624]: [good first verify][verify in Nightly only]

Status: Fixed. Works as expected.
You need to log in before you can comment on or make changes to this bug.