Closed Bug 888511 Opened 11 years ago Closed 11 years ago

We leak the Webapps::Launch DOM request

Categories

(Core Graveyard :: DOM: Apps, defect)

defect
Not set
normal

Tracking

(b2g18 affected)

RESOLVED FIXED
mozilla25
Tracking Status
b2g18 --- affected

People

(Reporter: justin.lebar+bug, Unassigned)

References

Details

(Whiteboard: [MemShrink])

Attachments

(3 files)

Or, I think we leak it. I think this is what's causing bug 851626, or at least part of it. Blocks a blocker.
Whiteboard: [MemShrink]
Blake pointed out that this bug also means that we don't fire onsuccess on the launch() DOMRequest. We should write a test for this.
blocking-b2g: --- → leo?
Whiteboard: [MemShrink]
Attached patch Patch, v1 (deleted) — Splinter Review
I haven't compiled this yet, because my tree is in a broken state at the moment. But something like this. We should check more generally that we're not leaking other DOM requests. How much you want to bet we are? At the very least we could implement a memory reporter for DOMRequestHelper that reports the number and type of requests it's storing. (I'm not sure we can easily report their size, since it's all JS.)
Whiteboard: [MemShrink]
Bug 888515 for the memory reporter.
This is a real leak, but if it doesn't fix bug 851626, it may not need to block. OTOH, this may be a necessary but not sufficient step to fixing bug 851626. Still figuring it out.
blocking-b2g: leo? → ---
Comment on attachment 769227 [details] [diff] [review] Patch, v1 This seems to fix the leak here, although it does not fix the bigger leak from the bug this one blocks.
Attachment #769227 - Attachment description: WIP, v1 → Patch, v1
Attachment #769227 - Flags: review?(21)
Another effect of this bug is that we never fire onsuccess for a launch() DOMRequest. I'd be happy to write a test for this, but I couldn't find any tests calling this method, and I was not ambitious enough to try to write one from scratch, since I don't know how to set up the proper environment and so on.
Comment on attachment 769227 [details] [diff] [review] Patch, v1 Review of attachment 769227 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Justin. For the tests, we have an ongoing effort to add a lot new ones for the dom/apps code. I'll take care of adding one for this bug.
Attachment #769227 - Flags: review?(21) → review+
Thanks, Fabrice. (Welcome back!) Note that although this patch applies to b2g18, it does not fix the bug there. That's because b2g18 does not send the Launch:OK message. I'm not sure yet if we need to fix this on b2g18 in order to fix bug 851626.
https://hg.mozilla.org/projects/birch/rev/45538bf7de97 We need to figure out if we need to port this to b2g18. The easiest thing to do is probably to run the test on trunk with and without this patch. mikeh, I'm happy to test if you teach me how.
Flags: needinfo?(mhabicher)
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 770558 [details] [diff] [review] Follow-up: Fire onsuccess properly in Webapp.js after launching an app. Review of attachment 770558 [details] [diff] [review]: ----------------------------------------------------------------- Blame the reviewer!
Attachment #770558 - Flags: review?(fabrice) → review+
jlebar, I ran the Camera<-->Gallery stress test last night with the above DOMRequest leakage fixes, and the device still hits the OoM condition. The global compartment is (still) disabled, and it looks like DOMRequestHelper.jsm is (still) eating up 25MiB.
Flags: needinfo?(mhabicher)
Comment 13 results we obtained using m-c/master trees.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: