1020196 - Allow using IndexedDB in nested oop iframe

Status: RESOLVED WORKSFORME

(Core :: Storage: IndexedDB, defect, P3)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

Currently TabParent::RecvPIndexedDBConstructor

  if (!IndexedDatabaseManager::IsMainProcess()) {
    NS_RUNTIMEABORT("Not supported yet!");
  }

Do we really need the window in IDBFactory or QuotaManager? Could we move PIndexedDB to PContent?

Comment 1

[Shian-Yow Wu [:swu]]

Attachment: Part 1: Allow PContent to create IndexedDB from a window.

WIP patch that moved PIndexedDB from PBrowser to PContent.

TODO: permission check on the origin.

Comment 2

[Shian-Yow Wu [:swu]]

Attachment: Part 2: Remove PIndexedDB from PBrowser.

Comment 3

[Jan Varga [:janv]]

You should really talk to Ben Turner, this clashes with IndexedDB on PBackground work he's been doing.

Comment 4

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

Hey guys, unfortunately the new indexedDB stuff (on the 'jamun' project branch) is getting pretty close to landing... It will no longer be tied to PContent/PBrowser there.

Comment 5

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

Sorry Shian-Yow, I didn't realize you were working on this :(

Comment 6

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

This is why you really should talk to module owners/peers before you start writing large pieces of code.

Comment 7

[Shian-Yow Wu [:swu]]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #4)
> Hey guys, unfortunately the new indexedDB stuff (on the 'jamun' project
> branch) is getting pretty close to landing... It will no longer be tied to
> PContent/PBrowser there.

Thanks for this info!

Comment 8

[Shian-Yow Wu [:swu]]

Is the understanding correct that once IndexedDB stuff moved from PContent/PBrowser to PBackground, we got IndexedDB working in nested OOP iframe?

Comment 9

[Kan-Ru Chen [:kanru] (UTC+9)]

I talked to Ben about this when he came to Taipei last time. We didn't expect the new IndexedDB on PBackground to land so soon so we went ahead to move PIndexedDB from PBrowser to PContent.

From a quick grep of the new code, it seems TabChild is only used to do permission check as in the old code. Fortunately this is the hard part that this bug has to address. Move PIndexedDB from PBrowser to PContent was relatively easy so I think we didn't waste too much time on it.

Comment 10

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

(In reply to Shian-Yow Wu [:swu] (Away 8/7 ~ 8/8) from comment #8)
> Is the understanding correct that once IndexedDB stuff moved from
> PContent/PBrowser to PBackground, we got IndexedDB working in nested OOP
> iframe?

Correct, the 'jamun' branch should let indexedDB work from anywhere (though, I don't believe we have any kind of tests for the nested process scenario, so there could be bugs).

Comment 11

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

(In reply to Kan-Ru Chen [:kanru] from comment #9)
> I talked to Ben about this when he came to Taipei last time. We didn't
> expect the new IndexedDB on PBackground to land so soon so we went ahead to
> move PIndexedDB from PBrowser to PContent.

Yeah, we're charging ahead quickly now. I guess the question is how soon you need this to work. If at all possible I would recommend waiting for 'jamun' to merge.

Comment 12

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

Comment on attachment 8464596 [details] [diff] [review]
Part 1: Allow PContent to create IndexedDB from a window.

I don't see where you're doing any security checks on the info passed in from the child here. We have to be able to prevent a child process from accessing data that belongs to another origin.

Comment 13

[Jenny Liu[:jennyliu]]

We plan to land it by the end of v2.1 sprint 3 (August 29).

Comment 14

[Shian-Yow Wu [:swu]]

Attachment: Part 1-1: Permission check.

The patch adds permission check.

In mochitest test, the check is not enforeced because neither IsForBrowser() and IsForAppp() is true.  Need to find some way to test it.

Comment 15

[Shian-Yow Wu [:swu]]

Attachment: Part 1(v2): Allow PContent to create IndexedDB from a window.

The patch added permission check and I confirmed that the check works on mochitest with B2G emulator at least.

bent, could you help to review it?

Comment 16

[Shian-Yow Wu [:swu]]

Attachment: Part 2: Remove PIndexedDB from PBrowser.

Rebased.

Comment 17

[Shian-Yow Wu [:swu]]

The patch also depends on bug 1020157 to land.

Comment 18

[Shian-Yow Wu [:swu]]

Just found some errors on tbpl.

https://tbpl.mozilla.org/?tree=Try&rev=8032e174fc39

Comment 19

[Kai-Chih Hu [:khu]]

Use feature-b2g:2.2? rather than just 2.2.

Comment 20

[Shian-Yow Wu [:swu]]

Tested with the new mochitest option "--nested" in bug 1038620, and all the IndexedDB cases in dom/indexedDB/test/ are passed.  Also tested with a gaia app to access IndexedDB by nested content process, and it's working properly.

But due to permission prompts for IndexedDB are being reworked a little still in bug 1083927, we'll need to test it again after it landed.  It's expected that the nested content process should fail with permission prompt when opening by |indexedDB.open("foo", { storage: "persistent" });| for explicit permanent storage requests.

Comment 21

[Shian-Yow Wu [:swu]]

In mochitest test cases for indexedDB, the case dom/indexedDB/test/test_persistenceType.html will open
the database with persistence storage type, and we expect that permission prompt is required.
However, due to MOZ_CHILD_PERMISSION is disabled, it won't actually run through the permission check procedures through child process.

The MOZ_CHILD_PERMISSIONS in http://dxr.mozilla.org/mozilla-central/source/configure.in#5705 is only enabled for B2G.  But it's not passed to indexedDB even when enabling it by removing the B2G check there.  Is this supposed to be an issue?

Comment 22

[Jan Varga [:janv]]

(In reply to Shian-Yow Wu [:swu] from comment #21)
> In mochitest test cases for indexedDB, the case
> dom/indexedDB/test/test_persistenceType.html will open
> the database with persistence storage type, and we expect that permission
> prompt is required.
> However, due to MOZ_CHILD_PERMISSION is disabled, it won't actually run
> through the permission check procedures through child process.
> 
> The MOZ_CHILD_PERMISSIONS in
> http://dxr.mozilla.org/mozilla-central/source/configure.in#5705 is only
> enabled for B2G.  But it's not passed to indexedDB even when enabling it by
> removing the B2G check there.  Is this supposed to be an issue?

I'm not sure I understand what you are asking...
I applied the patch from bug 1038620 and ran:
./mach mochitest-plain --e10s --nested dom/indexedDB/test/test_persistenceType.html
The test passed, but I know that we set the "indexedDB" permission for all the tests to avoid the prompt. So I also tested w/o the permission set. In that case I got:
Hit MOZ_CRASH(Figure out security checks for bridged content!) at /Users/varga/Sources/Mozilla/dom/ipc/TabParent.cpp:855
There's also a problem with running the test with just --e10s (w/o the permission set). In that case, browser UI doesn't launch the prompt at all.

So I don't see how this all relates to MOZ_CHILD_PERMISSION.

Comment 23

[Shian-Yow Wu [:swu]]

(In reply to Jan Varga [:janv] from comment #22)
> I applied the patch from bug 1038620 and ran:
> ./mach mochitest-plain --e10s --nested
> dom/indexedDB/test/test_persistenceType.html
> The test passed, but I know that we set the "indexedDB" permission for all
> the tests to avoid the prompt.

This answers my question why the test was passed, which I earlier thought it was related
to MOZ_CHILD_PERMISSION.  Thank you!

Comment 24

[Shian-Yow Wu [:swu]]

Attachment: Manage PIndexedDBPermissionRequest by PContent instead of PBrowser.

Comment 25

[Shian-Yow Wu [:swu]]

I am not actively working on this, unassign myself.

Comment 26

[Andrew Sutherland [:asuth] (he/him)]

I think this was mooted by the move to IndexedDB over PBackground many, many years ago. We still do have policy limitations on third-party iframes that might preclude IndexedDB running, but that's only a policy decision. If we're seeing weirdness related to fission, please file a new bug, although again, I'd expect it to be a privacy decision preventing use, not implementation.