888511 - We leak the Webapps::Launch DOM request

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Justin Lebar (not reading bugmail)]

Or, I think we leak it.  I think this is what's causing bug 851626, or at least part of it.

Blocks a blocker.

Comment 1

[Justin Lebar (not reading bugmail)]

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.

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1

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.)

Comment 3

[Justin Lebar (not reading bugmail)]

Bug 888515 for the memory reporter.

Comment 4

[Justin Lebar (not reading bugmail)]

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.

Comment 5

[Justin Lebar (not reading bugmail)]

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.

Comment 6

[Justin Lebar (not reading bugmail)]

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

[[:fabrice] Fabrice Desré]

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.

Comment 8

[Justin Lebar (not reading bugmail)]

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.

Comment 9

[Justin Lebar (not reading bugmail)]

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.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/45538bf7de97

Comment 11

[Justin Lebar (not reading bugmail)]

Attachment: Follow-up: Fire onsuccess properly in Webapp.js after launching an app.

Oops.

Comment 12

[[:fabrice] Fabrice Desré]

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!

Comment 13

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: about-memory report with DOMRequest patches applied

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.

Comment 14

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment 13 results we obtained using m-c/master trees.

Comment 15

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/25654c97929b

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/25654c97929b