756645 - Implement "indexedDB jars" for apps

Status: RESOLVED FIXED

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

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think this should be relatively easy to do now that bug 754141 is fixed. We just need to add the app-key to the origin.

Comment 1

[Andrew Overholt [:overholt]]

Mounir and Ben can fight over who gets this.  I'll assign it to Mounir for now :)

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Patch

That's quite simple actually: instead of getting the origin from nsContentUtils::GetASCIIOrigin, we just get it from nsIPrincipal::GetExtendedOrigin. This is also returning a puny-encoded origin.
I haven't renamed all the variables and stuff though.

I also removed a call to GetASCIIOriginFromWindow() that was obviously not used.

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: Patch + Tests

Jonas, could you review the tests. Ben reviewed the code.

Comment 4

[Mounir Lamouri (:mounir)]

Attachment: Patch + Tests

Those ones should pass try.

Comment 5

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

Comment on attachment 645465 [details] [diff] [review]
Patch + Tests

Review of attachment 645465 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/indexedDB/test/file_app_isolation.js
@@ +96,5 @@
> +    if (data.app || data.browser) {
> +      iframe.addEventListener('mozbrowsershowmodalprompt', function(e) {
> +        is(e.detail.message, 'success', 'test number ' + i);
> +
> +//        document.getElementById('content').removeChild(iframe);

Comment here that you're waiting for the other bug to be fixed.

Comment 6

[Mounir Lamouri (:mounir)]

So tests are green on try except that the OOP one is leaking. It's not because of the code change but very likely the browser element and less likely IndexedDB code. IOW, something is leaking and that test is making that obvious.

Given that we are going to ship B2G with OOP mozapps/browsers using IndexedDB, we will need to fix that leak. However, should we block that patch until the leak is fixed or should we push the patch with the OOP test disabled on debug so we can still test the behaviour but don't get failures for the leak?

Try results: https://tbpl.mozilla.org/?tree=Try&rev=190a90c10a4b

Comment 7

[Justin Lebar (not reading bugmail)]

> However, should we block that patch until the leak is fixed or should we push the patch 
> with the OOP test disabled on debug so we can still test the behaviour but don't get 
> failures for the leak?

Mounir cc'ed me, so presumably he would like my opinion?  I think we should leave this up to bent and co, but my feeling is that we should at last try to debug the leak, because otherwise, there's a distinct possibility we won't fix it.

Comment 8

[Andrew Overholt [:overholt]]

Can we land this and file a followup bug to fix the leak(s)?

Comment 9

[Mounir Lamouri (:mounir)]

The leak isn't happening any more \o/

https://hg.mozilla.org/mozilla-central/rev/4bb90f8c6909

Comment 10

[Ryan VanderMeulen [:RyanVM]]

This was backed out due to Android M3 timeouts in test_advance.html.
https://hg.mozilla.org/mozilla-central/rev/29ca472bf2d2

One example:
https://tbpl.mozilla.org/php/getParsedLog.php?id=14655303&tree=Firefox

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d192fe240461
https://hg.mozilla.org/mozilla-central/rev/4496025f9d35

Comment 12

[Jason Smith [:jsmith]]

This looks testable from an end-user perspective, but I need clarification on what exactly is being tested here. Looks like the testing would be similar to what was tested on desktop for the origin rules for indexedDB access for apps. Is that correct?

Comment 13

[Mounir Lamouri (:mounir)]

It is indeed testable from an end-user perspective but we have automated tests for that so I think it's not worth doing manual testings.

Comment 14

[Jason Smith [:jsmith]]

(In reply to Mounir Lamouri (:mounir) from comment #13)
> It is indeed testable from an end-user perspective but we have automated
> tests for that so I think it's not worth doing manual testings.

Okay sounds good.