1149420 - IndexedDB permission prompt does not work in e10s tabs.

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Jukka Jylänki]

STR:

1. Visit http://clb.demon.fi/dump/html5_popups/?persistent
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 (clb.demon.fi) 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.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

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.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Possible patch for Android.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

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 4

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

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.

Comment 5

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

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)

Comment 6

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

Blake, you should be able to enable the prompt tests for e10s now, remove "e10s" from http://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/test/browser.ini#2 I think? (You might have to disable some of the others individually)

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: With comments addressed

I combined the two patches into one. I'm pushing this to try now.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4c2411fccf9

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

I filed bug 1153491 per comment 7.

Comment 11

[Carsten Book [:Tomcat]]

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

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 13

[Andrea Marchesini [:baku]]

Comment on attachment 8591168 [details] [diff] [review]
With comments addressed

This is a dependence for bug 1151495.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e8c791225ca0

Comment 16

[Moin Shaikh]

QA Whiteboard [bugday-20150624]: [good first verify][verify in Nightly only]

Status: Fixed. Works as expected.