901416 - Increase Observer of "network-activity-blip-[upload|download]" when screen on/off

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[jeremy.kim.leo [:jeremykimleo]]

Version Info>
project b2g/gaia/
45f6a739b09292e16717fb21003386c914ca29c2 Bug 901044 - Everything.me needs additional strings for the search bar. r=pike

project b2g/gecko/
8878e8a4636c58227856ee2b76fc7d95a4cc9709 Bug 739681 - Allow DumpHeapComplete to print unreachable objects (r=mccr8 a=leo+)

STR>
1. enable Usage
2. turn on/off screen by power key
3. if #2 is over 200 times, target becomes extremely slow.

window object has a many observer regarding as "network-activity-blip-[upload|download]" when screen on/off.
it caused overhead of processing when target is doing network operation.

Comment 1

[jeremy.kim.leo [:jeremykimleo]]

Attachment: video of repro

00:15" - push home-button. no response
00:31" - push power-key. no response of lockscreen

Comment 2

[jeremy.kim.leo [:jeremykimleo]]

it can reproduce 100%. and if it is this situation. user can't do anything.

Comment 3

[Jinyoon]

This issue is originated from tester's normal behavior as end user.
- He used browser/youtube or other apps as end-user (not a stress test) for one or two days and then his devcie suddelny became slow.

And we found the sequence to reproduce as the above.

Comment 4

[jeremy.kim.leo [:jeremykimleo]]

dear. mounir,  can you help this?

Comment 5

[jeremy.kim.leo [:jeremykimleo]]

Attachment: proposal V1

i recomands it should fix like this.

globla variabls blocks AddObserver to regist one more.

Comment 6

[jeremy.kim.leo [:jeremykimleo]]

to remove Observer of network-activity-blip-[upload/download], it should call nsGlobalWindow::DisableNetworkEvent(uint32_t aType)

but, it isn't called because nsEventListenerManager::RemoveEventListener() never has typeCount == 0
https://hg.mozilla.org/releases/mozilla-b2g18/file/895f4b058c60/content/events/src/nsEventListenerManager.cpp#l468

Comment 7

[jeremy.kim.leo [:jeremykimleo]]

please check this.

Comment 8

[Justin Lebar (not reading bugmail)]

I think this is the right change; it looks like we call EnableNetworkEvent every time someone adds a network-event eventlistener on the window (see nsEventListenerManager.cpp).  I think the code here was written with the (incorrect) assumption that we get one DisableNetworkEvent call for every EnableNetworkEvent call.

Thanks, Jeremy.

Comment 10

[Justin Lebar (not reading bugmail)]

I'll push this today.

Comment 11

[Justin Lebar (not reading bugmail)]

Attachment: Patch for checkin

Trees are closed, let's just use checkin-needed.

This is the same as "proposal V1", except I changed the names of the variables and added commit headers.

There are merge conflicts when applying this to b2g18, but only in context.
That is to say, you should be able to uplift this without issue.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c5ed02feb7f4

Comment 13

[Ryan VanderMeulen [:RyanVM]]

And a follow-up fix for Werror bustage.
https://hg.mozilla.org/integration/b2g-inbound/rev/6d1b36d14a76

https://tbpl.mozilla.org/php/getParsedLog.php?id=26282437&tree=B2g-Inbound

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c5ed02feb7f4
https://hg.mozilla.org/mozilla-central/rev/6d1b36d14a76

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/88d329180da4

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/88d329180da4

Comment 17

[Justin Lebar (not reading bugmail)]

I filed bug 903290 on adding instrumentation which will make bugs like this easier to find.  Thanks again for finding this one.

Comment 18

[Sarah Parsons]

Unable to reproduce on Leo Build ID: 20130816041203 verifying fixed on b2g18

Environmental Variables
Build ID: 20130816041203
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/09a8c5b6b287
Gaia: 6040508a535b587d4155356a47b22d7bd9ea4de7
Platform Version: 18.1
RIL Version: 01.01.00.019.164

Comment 19

[Justin Lebar (not reading bugmail)]

Someone with the IRC nick sireesha has pinged me a few times about this bug.  Unfortunately these pings have been coming at around 11:30p my time, so I haven't been able to respond.

> <sireesha> After this fix the Usage widget data updation become very slow
> <sireesha> Please refer comment 16 in https://bugzilla.mozilla.org/show_bug.cgi?id=900923
> <sireesha> So i need information related to "How frequently network events are send to system 
> application"

I guess sireesha is trying to weigh the upside of this patch, since she believes there is a downside to the patch.

It looks like the fix here might have exposed a bug in the cost control app, where the activity threshold was set too high, because we were over-firing network-activity-blip events.

This patch is an important main-process memory leak fix.  If you don't take this fix in Leo, we cannot be held responsible for bugs where the phone becomes unresponsive after it is used for a long time.

The appropriate thing to do at this time is to file a bug which blocks this one, describing the regression resulting from this patch.  Once we understand the situation, we can weigh our options as far as backing this patch out or fixing the regression.