Last Comment Bug 756645 - Implement "indexedDB jars" for apps
: Implement "indexedDB jars" for apps
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla18
Assigned To: Mounir Lamouri (:mounir)
: Jason Smith [:jsmith]
Mentors:
Depends on: 775861
Blocks: app-data-jars
  Show dependency treegraph
 
Reported: 2012-05-18 14:35 PDT by Jonas Sicking (:sicking) PTO Until July 5th
Modified: 2012-09-02 08:01 PDT (History)
11 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+


Attachments
Patch (1.78 KB, patch)
2012-07-20 17:46 PDT, Mounir Lamouri (:mounir)
bent.mozilla: review+
Details | Diff | Splinter Review
Patch + Tests (10.11 KB, patch)
2012-07-22 17:37 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch + Tests (12.32 KB, patch)
2012-07-24 13:30 PDT, Mounir Lamouri (:mounir)
bent.mozilla: review+
Details | Diff | Splinter Review

Description Jonas Sicking (:sicking) PTO Until July 5th 2012-05-18 14:35:37 PDT
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] 2012-06-20 08:04:59 PDT
Mounir and Ben can fight over who gets this.  I'll assign it to Mounir for now :)
Comment 2 Mounir Lamouri (:mounir) 2012-07-20 17:46:58 PDT
Created attachment 644541 [details] [diff] [review]
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) 2012-07-22 17:37:19 PDT
Created attachment 644805 [details] [diff] [review]
Patch + Tests

Jonas, could you review the tests. Ben reviewed the code.
Comment 4 Mounir Lamouri (:mounir) 2012-07-24 13:30:28 PDT
Created attachment 645465 [details] [diff] [review]
Patch + Tests

Those ones should pass try.
Comment 5 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-07-24 13:41:59 PDT
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) 2012-07-25 10:27:03 PDT
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) 2012-07-25 10:53:15 PDT
> 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] 2012-08-09 11:24:53 PDT
Can we land this and file a followup bug to fix the leak(s)?
Comment 9 Mounir Lamouri (:mounir) 2012-08-23 14:10:23 PDT
The leak isn't happening any more \o/

https://hg.mozilla.org/mozilla-central/rev/4bb90f8c6909
Comment 10 Ryan VanderMeulen [:RyanVM] 2012-08-24 03:36:32 PDT
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 12 Jason Smith [:jsmith] 2012-09-01 07:54:21 PDT
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) 2012-09-02 08:01:03 PDT
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] 2012-09-02 08:01:46 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.