Closed
Bug 888511
Opened 11 years ago
Closed 11 years ago
We leak the Webapps::Launch DOM request
Categories
(Core Graveyard :: DOM: Apps, defect)
Core Graveyard
DOM: Apps
Tracking
(b2g18 affected)
RESOLVED
FIXED
mozilla25
Tracking | Status | |
---|---|---|
b2g18 | --- | affected |
People
(Reporter: justin.lebar+bug, Unassigned)
References
Details
(Whiteboard: [MemShrink])
Attachments
(3 files)
(deleted),
patch
|
fabrice
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
fabrice
:
review+
|
Details | Diff | Splinter Review |
(deleted),
application/x-zip-compressed
|
Details |
Or, I think we leak it. I think this is what's causing bug 851626, or at least part of it.
Blocks a blocker.
Updated•11 years ago
|
Whiteboard: [MemShrink]
Reporter | ||
Comment 1•11 years ago
|
||
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]
Reporter | ||
Comment 2•11 years ago
|
||
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.)
Reporter | ||
Updated•11 years ago
|
Whiteboard: [MemShrink]
Reporter | ||
Comment 3•11 years ago
|
||
Bug 888515 for the memory reporter.
Reporter | ||
Comment 4•11 years ago
|
||
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? → ---
Reporter | ||
Comment 5•11 years ago
|
||
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)
Reporter | ||
Comment 6•11 years ago
|
||
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 7•11 years ago
|
||
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+
Reporter | ||
Comment 8•11 years ago
|
||
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.
status-b2g18:
--- → affected
Reporter | ||
Comment 9•11 years ago
|
||
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)
Comment 10•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Reporter | ||
Updated•11 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 12•11 years ago
|
||
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+
Comment 13•11 years ago
|
||
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 14•11 years ago
|
||
Comment 13 results we obtained using m-c/master trees.
Reporter | ||
Comment 15•11 years ago
|
||
Comment 16•11 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•