Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Implement "indexedDB jars" for apps

RESOLVED FIXED in mozilla18

Status

()

Core
DOM: IndexedDB
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sicking, Assigned: mounir)

Tracking

(Blocks: 1 bug)

Trunk
mozilla18
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-kilimanjaro:+, blocking-basecamp:+)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

12.32 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
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.
No longer blocks: 756644
Blocks: 756644
blocking-basecamp: --- → +
blocking-kilimanjaro: --- → +
Mounir and Ben can fight over who gets this.  I'll assign it to Mounir for now :)
Assignee: nobody → mounir
Depends on: 775861
Assignee: mounir → jonas
(Assignee)

Updated

5 years ago
Assignee: jonas → mounir
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 2

5 years ago
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.
Attachment #644541 - Flags: review?(bent.mozilla)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
Attachment #644541 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 3

5 years ago
Created attachment 644805 [details] [diff] [review]
Patch + Tests

Jonas, could you review the tests. Ben reviewed the code.
Attachment #644541 - Attachment is obsolete: true
Attachment #644805 - Flags: review?(jonas)
(Assignee)

Comment 4

5 years ago
Created attachment 645465 [details] [diff] [review]
Patch + Tests

Those ones should pass try.
Attachment #644805 - Attachment is obsolete: true
Attachment #644805 - Flags: review?(jonas)
Attachment #645465 - Flags: review?(bent.mozilla)
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.
Attachment #645465 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 6

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

Updated

5 years ago
Priority: -- → P1
Can we land this and file a followup bug to fix the leak(s)?
(Assignee)

Comment 9

5 years ago
The leak isn't happening any more \o/

https://hg.mozilla.org/mozilla-central/rev/4bb90f8c6909
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: in-testsuite+
Target Milestone: mozilla17 → ---
(Assignee)

Updated

5 years ago
Flags: in-testsuite+
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/d192fe240461
https://hg.mozilla.org/mozilla-central/rev/4496025f9d35
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
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?
QA Contact: jsmith
Whiteboard: [qa?]
(Assignee)

Comment 13

5 years ago
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.
(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.
Whiteboard: [qa?] → [qa-]
You need to log in before you can comment on or make changes to this bug.