IndexedDB permission prompt does not work in e10s tabs.

RESOLVED FIXED in Firefox 39

Status

()

Core
DOM: IndexedDB
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Jukka Jylänki, Assigned: mrbkap)

Tracking

40 Branch
mozilla40
x86_64
Windows 8.1
Points:
---

Firefox Tracking Flags

(e10sm6+, firefox39 fixed, firefox40 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Updated

3 years ago
tracking-e10s: --- → ?
(Reporter)

Updated

3 years ago
Version: Trunk → 40 Branch
(Assignee)

Updated

3 years ago
Assignee: nobody → mrbkap
tracking-e10s: ? → m6+
(Assignee)

Comment 1

3 years ago
Created attachment 8590574 [details] [diff] [review]
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.
Attachment #8590574 - Flags: review?(bent.mozilla)
(Assignee)

Comment 2

3 years ago
Created attachment 8590575 [details] [diff] [review]
Possible patch for Android.
Attachment #8590575 - Flags: review?(mark.finkle)
(Assignee)

Comment 3

3 years ago
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 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)
(Assignee)

Comment 7

3 years ago
(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.
(Assignee)

Comment 8

3 years ago
Created attachment 8591168 [details] [diff] [review]
With comments addressed

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+
(Assignee)

Comment 10

3 years ago
I filed bug 1153491 per comment 7.
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c76e6b6d4591
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Blocks: 1151495
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]

Comment 16

3 years ago
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.