836654 - Hold processes in FOREGROUND or FOREGROUND_HIGH when they have a pending system message

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Justin Lebar (not reading bugmail)]

I think we need this for bug 836199 and kin.

Cribbing from bug 836199 comment 1, here's what currently happens when we receive a call:

  a) Gecko receives a call.
  b) Gecko asks Gaia to start the phone app in the background.
  c) The phone app starts up in the background and receives a message indicating that there's a call pending.
  d) The phone app opens an attention window, which is in the foreground.

We can elevate the phone app into the foreground for a few seconds, but the best we can do right now is send it into the background on a timeout.  That means that if the phone app takes too long to start up (for whatever reason), we drop the call.  Or we don't play the alarm.

I think the right thing to do here is, for "critical" system messages, to wait until the app has a chance to receive its system message before sending it to the background.  (When the app receives the system message, it may open a window, thus locking itself in the foreground.)  In fact, the app should probably run with a priority above FOREGROUND, so we ensure it doesn't get killed in the meantime.

Comment 1

[Justin Lebar (not reading bugmail)]

Talking this over with vingtetun, I'm thinking that the following is a good solution to this:

* When we dispatch a system message to an app, we "grab a CPU wake lock on behalf of that app", using an API which doesn't exist yet.  We release this wake lock when the app finishes handling the system message.

* When the app receives the system message, it may choose to grab a CPU wake lock itself.

* Different apps get different priorities when they hold the CPU wake lock.  The telephony app gets HIGHPRI_LOCKED or something, which falls between the B2G root process and foreground processes.  Same for the alarm app.  In contrast, a "regular" app gets BACKGROUND_PERCEIVABLE if it's in the background, or stays at regular FOREGROUND if it's already in the fg.

I think this would solve the problem I identified in bug 836638 comment 5: Right now if the alarm app crashes after we open it but before it handles its system message, I think we hold the CPU lock forever.  That's because we're doing a hacky job of implementing the "grab the CPU lock on behalf of this process" bit, which in the case of process crashes.  (Regular CPU locks work properly in this case.)

Comment 2

[Andrew Overholt [:overholt]]

Blocks blockers to tef+

Comment 3

[Gene Lian (I already quit Mozilla)]

(In reply to Justin Lebar [:jlebar] from comment #1)
> I think this would solve the problem I identified in bug 836638 comment 5:
> Right now if the alarm app crashes after we open it but before it handles
> its system message, I think we hold the CPU lock forever.

Actually, it won't happen. I remember we used to add an extra timer for insurance. Didn't we? Anyway, it sounds safer to grab the CPU lock by the process itself. :)

However, I think we still need to grab an inevitable CPU wake lock in the parent to make sure the system message has enough CPU resource to be safely sent/broadcasted to the child process(es) on time. Grabbing the CPU lock on behalf of apps might be too late because the app hasn't been launched yet.

Please correct me if I misunderstood.

Comment 4

[Justin Lebar (not reading bugmail)]

> I remember we used to add an extra timer for insurance.

Ah, right!  That was forward-thinking of us.  :)

> Grabbing the CPU lock on behalf of apps might be too late because the app hasn't been 
> launched yet.

I don't get what you mean -- if I grab the lock on behalf of the app /before/ I launch the app, then how can that be too /late/?  I'm getting the lock very /early/ in the cycle.

Comment 5

[Gene Lian (I already quit Mozilla)]

(In reply to Justin Lebar [:jlebar] from comment #4)
> > Grabbing the CPU lock on behalf of apps might be too late because the app hasn't been 
> > launched yet.
> 
> I don't get what you mean -- if I grab the lock on behalf of the app
> /before/ I launch the app, then how can that be too /late/?  I'm getting the
> lock very /early/ in the cycle.

Ha! I probably misunderstood. I originally thought you're thinking of letting the *child process* grab the CPU lock by itself. Somewhere in the SystemMessageManager for example.

So, you're talking about taking the app info into consideration when grabbing CPU wake lock in the parent, and different apps could have different priorities? For example, adding a new parameter |aAppInfo| or something like that:

  powerManagerService.newWakeLock("cpu", aAppInfo)

Comment 6

[Justin Lebar (not reading bugmail)]

> So, you're talking about taking the app info into consideration when grabbing CPU wake 
> lock in the parent, and different apps could have different priorities? 

Yes, exactly.  I'm not sure what the API would be, but something like what you've described.

Comment 7

[Andrew Overholt [:overholt]]

Who should be the assignee here?  jlebar?  clian?

Comment 8

[Justin Lebar (not reading bugmail)]

I'm working on this.  But there are two separate Gaia dependencies that I really need help with (see dep tree).  Plus Vivien has an alternative approach which involves mostly Gaia work.

Comment 9

[Justin Lebar (not reading bugmail)]

Vivien suggested an alternative here that I like much better than the approach I suggested in comment 1: Add the ability for apps to register a system-message handler that directly opens an attention screen.

We'll probably need to make some tweaks on the Gecko side to prioritize these attention windows more quickly and so on, but I think this Gaia change would make the necessary Gecko changes here much less invasive.

I think Fernando is working on this, but if there's a bug I haven't been cc'ed.

Comment 10

[Justin Lebar (not reading bugmail)]

I have a Gecko fix for this (only minor Gaia changes required) that I'm almost ready to post; I just need to clean it up.  It seems to help a lot, in my testing.

Comment 11

[Justin Lebar (not reading bugmail)]

Attachment: Part 1: Don't require the "power" permission to do GetWakeLockInfo from a child process.

I don't see any reason not to let child processes inspect the current state of a wake lock.

Comment 12

[Justin Lebar (not reading bugmail)]

Attachment: Part 2: Add PROCESS_PRIORITY_FOREGROUND_HIGH.

Comment 13

[Justin Lebar (not reading bugmail)]

Attachment: Part 3: Modify hal to accept a ContentParent ID in ModifyWakeLock().

Also return the list of processes holding the lock in GetWakeLockInfo
and in the wake-lock changed notification.

Kan-Ru knows this code best, so he's probably the best person to review it.  But I imagine he's on vacation right now...

Comment 14

[Justin Lebar (not reading bugmail)]

Attachment: Part 4: Add PowerManagerService::NewWakeLockOnBehalfOfProcess.

Comment 15

[Justin Lebar (not reading bugmail)]

Attachment: Part 5: Add nsIMozBrowserFrame::isExpectingSystemMessage.

I'm happy to change the attribute name; it's currently "expecting-system-message".

Comment 16

[Justin Lebar (not reading bugmail)]

Attachment: Part 6: If a process is "critical" and holds the CPU wake lock, give it priority FOREGROUND_HIGH.

Comment 17

[Justin Lebar (not reading bugmail)]

Attachment: Part 7: Hold a CPU wake lock while a "critical" process that's expecting a system message loads.

Comment 18

[Justin Lebar (not reading bugmail)]

Attachment: Part 8: Inform the mozbrowser embedder when we're opening a frame that we expect to send a system message to.

Comment 19

[Justin Lebar (not reading bugmail)]

Attachment: Part 9: Don't acquire the CPU wake lock in the alarm service any longer.

We've removed the event that the AlarmService was listening to in order
to find out when we could drop the lock!

It's a bit scary to make this change, because if something is wrong with the
rest of the code, we'll be in a worse state than we were to begin with.
Perhaps we should leave this code in and continue sending the
finished-handling-system-message event, for now.  Or maybe we should go big or
go home.  Do you have a preference?

Comment 20

[Justin Lebar (not reading bugmail)]

Comment on attachment 712776 [details] [diff] [review]
Part 6: If a process is "critical" and holds the CPU wake lock, give it priority FOREGROUND_HIGH.

Note that this is a departure from our previous design for wake-locks.

Before, we impugned no semantics onto any particular wake lock.  It was entirely up to Gaia to give the wake locks meaning.

But now Gecko has hard-coded semantics for the "cpu" wake lock.

Comment 21

[Justin Lebar (not reading bugmail)]

Attachment: Part 7, v2: Hold a CPU wake lock while a "critical" process that's expecting a system message loads.

Missed a spot.

Comment 22

[Justin Lebar (not reading bugmail)]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8053

Pointer to Github pull-request

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 712775 [details] [diff] [review]
Part 5: Add nsIMozBrowserFrame::isExpectingSystemMessage.

r=me, I guess.  Setting attributes for this feels kinda heavyweight.... :(

Comment 24

[Justin Lebar (not reading bugmail)]

As a high-level overview, this patch queue works as follows.

* If an <iframe mozbrowser> with the "expecting-system-message" attribute is created, we acquire the CPU wake lock in the main process on behalf of that process.  The wake lock is released after the process handles the system message, or after 30s, whichever comes first.

* If an <iframe mozbrowser> with the mozapptype attribute set to "critical" holds the CPU wake lock, it gets priority FOREGROUND_HIGH.

* The Gaia patch gives the phone app and the alarm app "critical" status.

* So we should therefore be much less likely to drop calls or alarms.  I can't say that it never happens; I saw us drop an alarm once, and then wasn't able to reproduce it again.  But I cannot reproduce consistently, which is a big improvement.

Sorry this took me so long to get done.

Comment 25

[Justin Lebar (not reading bugmail)]

(In reply to Boris Zbarsky (:bz) from comment #23)
> Comment on attachment 712775 [details] [diff] [review]
> Part 5: Add nsIMozBrowserFrame::isExpectingSystemMessage.
> 
> r=me, I guess.  Setting attributes for this feels kinda heavyweight.... :(

Yeah; I'm not happy with it either.

I suppose once we have a dedicated element for mozbrowser, we'll be able to call a function to do this.  Right now we can't do that because BrowserElementParent.js doesn't get instantiated until after we insert the frame into the DOM, and by that point it's too late to set the new process's priority.

Comment 26

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'm working through a build issue (machine problem) that's preventing me from testing these patches; sorry.  Will review tomorrow morning my time.

Comment 27

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'll just quickly whinge that I don't like adding extra complexity to the memory management model in order to (apparently, mostly) work around gaia bugs.  But we're down to the wire and failing all memory tests.  Let's hope we can shed this in a v.next.

Comment 28

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 712771 [details] [diff] [review]
Part 1: Don't require the "power" permission to do GetWakeLockInfo from a child process.

(We need this to compute foreground priority in part 6.)

Comment 29

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 712773 [details] [diff] [review]
Part 3: Modify hal to accept a ContentParent ID in ModifyWakeLock().

Throughout this patch, we're using -1 with uint64_t, which is bad
style even if it compiles and works everywhere.  Please use a
different sentinel value or switch to int64_t.

>diff --git a/hal/Hal.h b/hal/Hal.h

>+ * In most cases, you shouldn't need to pass the aProcessID argument; the
>+ * default of -1 is probably what you want.

(It looks like the use case here is part 4.)

>+ *
>  * @param aTopic        lock topic

Not your fault, but |topic| isn't a great name for "wake lock type".
Would appreciate it we fixed this while we're here, but not required.

>diff --git a/hal/HalTypes.h b/hal/HalTypes.h

>+#include "Observer.h"

Do we need this change?

r=me with those addressed.

Comment 30

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 712774 [details] [diff] [review]
Part 4: Add PowerManagerService::NewWakeLockOnBehalfOfProcess.

More uint64_t / -1 issues here.

I understand the mechanics of this code well enough to r+, but we
should let Kan-Ru provide feedback if he wishes.

r=me with that

Comment 31

[Justin Lebar (not reading bugmail)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #29)
> Comment on attachment 712773 [details] [diff] [review]
> Part 3: Modify hal to accept a ContentParent ID in ModifyWakeLock().
> 
> Throughout this patch, we're using -1 with uint64_t, which is bad
> style even if it compiles and works everywhere.  Please use a
> different sentinel value or switch to int64_t.

I can't use 0, because that means "main process".

Do you want me to attach a patch switching all of the IDs to int64_t?  Or would you feel OK if I added ContentChild::kUnknownID == -1?  Switching just some of the ID variables to int64_t and leaving others at uint64_t feels messy to me.

> >diff --git a/hal/HalTypes.h b/hal/HalTypes.h
> >+#include "Observer.h"
> 
> Do we need this change?

I'm not 100% sure if my code compiles without that include, but HalTypes.h uses the Observer class without including Observer.h, so this change seems correct to me.

Comment 32

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 712776 [details] [diff] [review]
Part 6: If a process is "critical" and holds the CPU wake lock, give it priority FOREGROUND_HIGH.

Looks good.  I assume the names will change a bit when we untie the
dialer from the cpu lock, but I don't need to see that.

Comment 33

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 712784 [details] [diff] [review]
Part 7, v2: Hold a CPU wake lock while a "critical" process that's expecting a system message loads.

I assume the naming here will change a bit too, but I also don't care
to see those changes.

>diff --git a/dom/ipc/ContentChild.cpp b/dom/ipc/ContentChild.cpp
 
>+class SystemMessageHandledObserver MOZ_FINAL : public nsIObserver

 -> fabrice

>diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp

>+/*static*/ ProcessPriority
>+ContentParent::GetInitialProcessPriority(nsIDOMElement* aFrameElement)
>+{

>+    if (!aFrameElement) {
>+        return PROCESS_PRIORITY_FOREGROUND;
>+    }

Can this ever happen?

>+    nsAutoString appType;
>+    nsCOMPtr<Element> frameElement = do_QueryInterface(aFrameElement);
>+    frameElement->GetAttr(kNameSpaceID_None, nsGkAtoms::mozapptype, appType);

Do we only honor this attribute when set by "embed-apps" contexts?  We
don't want wikipedia using this, e.g.  Not an issue for 1.0.x though.

>+class SystemMessageHandledListener MOZ_FINAL
>+    : public nsITimerCallback
>+    , public LinkedListElement<SystemMessageHandledListener>

 -> fabrice

>diff --git a/dom/ipc/PContent.ipdl b/dom/ipc/PContent.ipdl

>+    // Notify the parent that the child has finished handling a system message.
>+    async SystemMessageHandled();

I'm not sure this is correct, but -> fabrice :).

The remaining dom/ipc changes look OK to me with comments above
addressed.

Comment 34

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 712779 [details] [diff] [review]
Part 9: Don't acquire the CPU wake lock in the alarm service any longer.

Righteous!  This isn't my front yard but this is an easy to understand change.

Where's the XXX work going to be done?

Comment 35

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 712786 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8053

(Just as a reminder, we can't take this patch as-is.)

Comment 36

[Justin Lebar (not reading bugmail)]

>>+    if (!aFrameElement) {
>>+        return PROCESS_PRIORITY_FOREGROUND;
>>+    }
>
> Can this ever happen?

Looks like I was being paranoid.

> Do we only honor this attribute when set by "embed-apps" contexts?

That attribute only comes into effect when we're creating a remote frame, which can only happen in embed-apps or mozbrowser contexts.  So yes, if I understand the question.

>>+    async SystemMessageHandled();
>
> I'm not sure this is correct, but -> fabrice :).

FWIW I had to make this change because the original code was using the message manager to deliver a message from the child --> parent, but the MM is JS-only, and I needed to listen from C++.  So instead I fire an observer and let the child send an IPDL message to the parent.  It's certainly ugly.

Comment 37

[Justin Lebar (not reading bugmail)]

> Where's the XXX work going to be done?

Oops; I meant to do that, just forgot.

Comment 38

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Justin Lebar [:jlebar] from comment #31)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #29)
> > Comment on attachment 712773 [details] [diff] [review]
> > Part 3: Modify hal to accept a ContentParent ID in ModifyWakeLock().
> > 
> > Throughout this patch, we're using -1 with uint64_t, which is bad
> > style even if it compiles and works everywhere.  Please use a
> > different sentinel value or switch to int64_t.
> 
> I can't use 0, because that means "main process".
> 
> Do you want me to attach a patch switching all of the IDs to int64_t?  Or
> would you feel OK if I added ContentChild::kUnknownID == -1?  Switching just
> some of the ID variables to int64_t and leaving others at uint64_t feels
> messy to me.

Yes, symbolic constant would be great.

Comment 39

[Justin Lebar (not reading bugmail)]

> Or would you feel OK if I added ContentChild::kUnknownID == -1?

It looks like we already have CONTENT_PARENT_UNKNOWN_CHILD_ID, defined to -1.  I'm just going to use that, unless you prefer I do something else.

Comment 40

[Justin Lebar (not reading bugmail)]

Attachment: Part 0: Add constants CONTENT_PROCESS_{UNKNOWN_ID,MAIN_ID} to HalTypes.h and remove the equivalent constants from ContentParent.h.

Comment 41

[Justin Lebar (not reading bugmail)]

Attachment: Part 10: Take the CPU wake lock only after sending down the mozapptype.

This prevents the child process from downgrading its CPU priority when
it notices that it has the CPU wake lock but no "critical" frames.

In this patch we also reduce the scope of a race condition which occurs
when we don't launch a new process for an app.  In this case, we still
need to set the child process's priority and then check that the process
is still alive.

Because this isn't a brand-new process, it's possible that the process
will downgrade its priority (e.g. due to a timer) after we check that
it's still alive and before it notices that we've acquired the CPU wake
lock on its behalf.  This patch does not resolve that race.

Comment 42

[Justin Lebar (not reading bugmail)]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8053

Pointer to Github pull-request

Comment 43

[Justin Lebar (not reading bugmail)]

Comment on attachment 713749 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8053

Sorry for asking for reviews so close to our deadline, guys.

Comment 44

[Justin Lebar (not reading bugmail)]

With these patches, when I receive a call and the dialier app is initially not alive, the dialier is in FOREGROUND_HIGH the entire time it's alive.

When I get an alarm and the alarm app is not initially alive, it is in FOREGROUND_HIGH until it shows the attention screen, at which point it downgrades itself to FOREGROUND.  We could acquire the high-priority wake-lock in the alarm app while the alarm is sounding, which would make it behave just like the dialer.

We still have some races with respect to delivering system messages to processes which are already alive.  These are tricky to resolve, but they probably don't block.

The only circumstances under which I can get the phone to not deliver a phone call are when we have multiple browser tabs all sitting in the foreground (bug 836647).  In this case, the phone app does not crash; it's just starved for CPU.  Once the tabs finish their business, we get a missed call notification.  :)

Comment 45

[Justin Lebar (not reading bugmail)]

If you'd like to test these patches, you can use my git branch: https://github.com/jlebar/mozilla-central/tree/process-priority-fixes

Comment 46

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 712772 [details] [diff] [review]
Part 2: Add PROCESS_PRIORITY_FOREGROUND_HIGH.

>diff --git a/b2g/app/b2g.js b/b2g/app/b2g.js

> // Niceness values (i.e., CPU priorities) for B2G processes.
> pref("hal.processPriorityManager.gonk.masterNice", 0);
>-pref("hal.processPriorityManager.gonk.foregroundNice", 1);
>+pref("hal.processPriorityManager.gonk.foregroundHighNice", 1);
>+pref("hal.processPriorityManager.gonk.foregroundNice", 2);

On second thought, (following up to IRC discussion), I think we should keep fg priority the same and make fghigh either same as master or same as fg.  I lean towards same as master but could be convinced otherwise.

I'm mostly worried about changing current app perf characteristics, and less about the process buster testcase.

Comment 47

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 713747 [details] [diff] [review]
Part 10: Take the CPU wake lock only after sending down the mozapptype.

>diff --git a/dom/alarm/AlarmService.jsm b/dom/alarm/AlarmService.jsm

>     // add the messages to be listened
>     const messages = ["AlarmsManager:GetAll",
>                       "AlarmsManager:Add",
>-                      "AlarmsManager:Remove",
>-                      "SystemMessageManager:HandleMessageDone"];
>+                      "AlarmsManager:Remove"];

Followup to part 9?  Otherwise I don't understand, and please r? fabrice :).

>diff --git a/dom/ipc/ProcessPriorityManager.cpp b/dom/ipc/ProcessPriorityManager.cpp

>-#if defined(ANDROID) && 0
>+#if defined(ANDROID) /*&& 0*/

Please remove for landing.

Nice catch.  Code looks good.  r=me with above.

Comment 48

[Justin Lebar (not reading bugmail)]

> Followup to part 9?

Oops; yeah, that should be in part 9.  It's just removing a message we're no longer listening for.  I'm not going to flag r? because it's a trivial change and I'd like to land this stuff now; hopefully Fabrice is OK with that. 

> Please remove for landing.

Thanks for catching that.  One of these days I'm going to fix our logging story...

Comment 49

[[:fabrice] Fabrice Desré]

(In reply to Justin Lebar [:jlebar] from comment #48)
> > Followup to part 9?
> 
> Oops; yeah, that should be in part 9.  It's just removing a message we're no
> longer listening for.  I'm not going to flag r? because it's a trivial
> change and I'd like to land this stuff now; hopefully Fabrice is OK with
> that. 

I am ok. go ahead!

Comment 50

[Justin Lebar (not reading bugmail)]

> On second thought, (following up to IRC discussion), I think we should keep fg priority the same and 
> make fghigh either same as master or same as fg.

Okay, I'll just change fg_high to == master.  I'm afraid of the fg_high processes starving the master process, but CPU priorities are not strict, and anyway we control the code of these fg_high processes.  Better to be conservative and not tweak the other priorities unnecessarily.

Comment 51

[Justin Lebar (not reading bugmail)]

Bombs away.

This needs to stay open until we land the Gaia bits, but landing on Gecko first lets us shake things out and also lets Gaia devs test using nightly Gecko builds.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc2132ad05a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/77b0d7480b4c
https://hg.mozilla.org/integration/mozilla-inbound/rev/235887eecb56
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea0b768ca1c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/1862aa4b43ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/17f9660c7a7b
https://hg.mozilla.org/integration/mozilla-inbound/rev/693868d079cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef4884e40bb4
https://hg.mozilla.org/integration/mozilla-inbound/rev/f951099c6a3c
https://hg.mozilla.org/integration/mozilla-inbound/rev/55f1f62f7477
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cf341d4ccbd

Comment 52

[Justin Lebar (not reading bugmail)]

Once we land the Gaia changes here, it would be extremely helpful if QA could test whether the bugs that this bug blocks are fixed.

Comment 53

[Justin Lebar (not reading bugmail)]

And hopefully this is the only build issue I have.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b04f5fd79d6

Comment 54

[Justin Lebar (not reading bugmail)]

I'm not sure if pushing at 3am was a brilliant idea or a terrible one.

Follow-up #2: https://hg.mozilla.org/integration/mozilla-inbound/rev/a74f08ec0e46

Also I didn't change the CPU priority prefs like I intended to, but at this point it seems pretty likely that I'm going to be backed out for reals, so I don't want to keep piling on.  I'll fix them first thing tomorrow.

Sorry for making a mess.

Comment 55

[Etienne Segonzac (:etienne)]

Comment on attachment 713749 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8053

r=me with nits addressed on the dialer part, I'll let Vivien do the system app part since I heard he has at least 1 comment.

Comment 56

[Ed Morley [:emorley]]

Push & followups backed out for mochitest-2 and mochitest-other failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=a74f08ec0e46

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=f48618e815d1

Comment 57

[Justin Lebar (not reading bugmail)]

I can't reproduce these test failures locally, so I'm bisecting on try.

https://tbpl.mozilla.org/?tree=Try&rev=45f07482b9c8
https://tbpl.mozilla.org/?tree=Try&rev=41e6b2b38fa9
https://tbpl.mozilla.org/?tree=Try&rev=861361d09eb8
https://tbpl.mozilla.org/?tree=Try&rev=a5347d6c4177

Comment 58

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 713749 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8053

I really not a fan of the hardcoded list but I won't block on that. I feel like we should check for the attention screen permission since this is certified app only.

Comment 59

[Justin Lebar (not reading bugmail)]

The mysterious test failures look like we maybe needed to clobber on m-i, since the try push passes.  That's rather odd, though...

Comment 60

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d87c3a49bff1
https://hg.mozilla.org/integration/mozilla-inbound/rev/e74011e49190
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb43c5972c2c
https://hg.mozilla.org/integration/mozilla-inbound/rev/6480e7d33c5a
https://hg.mozilla.org/integration/mozilla-inbound/rev/691f0a62b261
https://hg.mozilla.org/integration/mozilla-inbound/rev/63219dc42eac
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f8506404118
https://hg.mozilla.org/integration/mozilla-inbound/rev/41a1257b31cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/942cec35b0e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e64eb3c6c96
https://hg.mozilla.org/integration/mozilla-inbound/rev/832ac179e3d0
https://hg.mozilla.org/integration/mozilla-inbound/rev/4497d85b07f4

Comment 61

[Justin Lebar (not reading bugmail)]

Someone asked on IRC if we were ready for QA to test this.  The answer is, please wait until we've landed the relevant Gaia changes.  Also, there are a few follow-ups I'm still working through (see the bugs which this depends on).

Comment 62

[Justin Lebar (not reading bugmail)]

I'm still working through some issues in the Gaia code (in combination with the patch in bug 836647), so I'm not ready to land that bit just yet.  I'll try to make the changes requested in review and finish up my testing tomorrow.

Additionally there are some Gecko follow-ups that need to land.

Comment 63

[Gene Lian (I already quit Mozilla)]

Just wondering do we need to do the same trick for handling the incoming call [1] (remove the codes like alarm)? It is requesting a CPU wake lock with timer to address bug 830425.

Btw, awesome patches! Justin!

[1] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#1296

Comment 64

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d87c3a49bff1
https://hg.mozilla.org/mozilla-central/rev/e74011e49190
https://hg.mozilla.org/mozilla-central/rev/fb43c5972c2c
https://hg.mozilla.org/mozilla-central/rev/6480e7d33c5a
https://hg.mozilla.org/mozilla-central/rev/691f0a62b261
https://hg.mozilla.org/mozilla-central/rev/63219dc42eac
https://hg.mozilla.org/mozilla-central/rev/9f8506404118
https://hg.mozilla.org/mozilla-central/rev/41a1257b31cb
https://hg.mozilla.org/mozilla-central/rev/942cec35b0e4
https://hg.mozilla.org/mozilla-central/rev/5e64eb3c6c96
https://hg.mozilla.org/mozilla-central/rev/832ac179e3d0
https://hg.mozilla.org/mozilla-central/rev/4497d85b07f4

Comment 65

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/3de3a83f5bf2
https://hg.mozilla.org/releases/mozilla-b2g18/rev/a2396ec10915
https://hg.mozilla.org/releases/mozilla-b2g18/rev/946bda7e750a
https://hg.mozilla.org/releases/mozilla-b2g18/rev/17e04bbdcf5b
https://hg.mozilla.org/releases/mozilla-b2g18/rev/971705534b1e
https://hg.mozilla.org/releases/mozilla-b2g18/rev/1799c434ac71
https://hg.mozilla.org/releases/mozilla-b2g18/rev/66c893cf5c10
https://hg.mozilla.org/releases/mozilla-b2g18/rev/95baa4ffc85d
https://hg.mozilla.org/releases/mozilla-b2g18/rev/d2c83ced1c9d
https://hg.mozilla.org/releases/mozilla-b2g18/rev/e874d5ba8354
https://hg.mozilla.org/releases/mozilla-b2g18/rev/905002d0ddd1
https://hg.mozilla.org/releases/mozilla-b2g18/rev/6cfeb6a43942

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/f837b6cbfcb0
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/3e272554ca53
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/5adfa9343cf9
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/234706fc8304
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/60e567957b20
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/219ad5783ed2
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/18fb2979bce9
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/1a983f5a51bb
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/f12fc5b90c20
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/a52f0c8b7726
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/3e95c8e18f0f
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/2fcf30aa8412

Comment 66

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Thanks, jelebar.  removing qawanted

Comment 67

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Please add qawanted to bug 836647 when it's ready to test on the gaia end.  If you mark it needsinfo :nhirata it will get faster attention from me.

Comment 68

[Justin Lebar (not reading bugmail)]

Reopening because the Gaia bits haven't landed yet.

Note to self: Vivien asked in the github PR that instead of hardcoding a list of origins which are "critical" we use a permission on the app.  But doing that would put another call to Applications.getByManifestURL() on the app startup critical path.

So we said that I'd file a follow-up bug to remove the hardcoded list after he finishes his existing work to add a cache for the Applications.getByManifestURL business.

Comment 69

[Justin Lebar (not reading bugmail)]

I made a new pull request, this time against gaia/master.

I'm not totally confident I did this right, so I'm not merging it.  Could someone please double-check and then merge this?

https://github.com/mozilla-b2g/gaia/pull/8143

Do I need to make a separate PR in order to uplift to b2g18-v1.0.1?

Comment 70

[Justin Lebar (not reading bugmail)]

Note that based on my testing, the base issue here is not 100% fixed with these changes.   I'm still seeing situations where, when the phone is under load, the communications app drops down in priority.  I'm not sure why this is happening, but cjones and I think we can handle it as a follow-up.

Comment 71

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 712774 [details] [diff] [review]
Part 4: Add PowerManagerService::NewWakeLockOnBehalfOfProcess.

Review of attachment 712774 [details] [diff] [review]:
-----------------------------------------------------------------

Ha, I'm late to this party. LGTM

Comment 72

[Dale Harvey (:daleharvey)]

Merged in: https://github.com/mozilla-b2g/gaia/commit/dac3c41ee37d787a9483f41ea5c40a266f09ad59

Comment 73

[Justin Lebar (not reading bugmail)]

Dale merged this in to master (thanks, Dale)!  But neither of us is totally sure what we need to do to get this into the other branches.  Vivien, can you help?

https://github.com/mozilla-b2g/gaia/commit/dac3c41ee37d787a9483f41ea5c40a266f09ad59

It would be great to get some QA coverage on this issue.  See the bugs this blocks for various STR, but it would also be helpful if you could be creative in trying to break this.

Comment 74

[Justin Lebar (not reading bugmail)]

(In reply to Gene Lian [:gene] from comment #63)
> Just wondering do we need to do the same trick for handling the incoming
> call [1] (remove the codes like alarm)?

I think we can remove that code, but perhaps it's not something to do right now, because I don't think that code is hurting us, so fixing it would be unnecessary risk.  Would you mind filing a follow-up bug?

Comment 75

[Justin Lebar (not reading bugmail)]

Vivien, can please you advise what we need to do to get this patch uplifted to the correct Gaia repositories?

Comment 76

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Justin Lebar [:jlebar] from comment #75)
> Vivien, can please you advise what we need to do to get this patch uplifted
> to the correct Gaia repositories?

I wonder if we can open a bug against Boot2Gecko::Gaia so I can approve it (there is no flags for that when the bug is not in Boot2Gecko::Gaia*). But let's ask akeybl to see if there is a better way to resolve this when a bug contains patches for both Gecko and Gaia.

Comment 77

[Justin Lebar (not reading bugmail)]

This is already blocking+, so if it has your verbal approval, can we say that's sufficient and just land it?

Comment 78

[Justin Lebar (not reading bugmail)]

Can we please, please merge these Gaia changes?  They've been waiting on a technicality for far too long.  This TEF blocker is not fixed for v1 until the Gaia changes land on gaia-v1.

Comment 79

[Justin Lebar (not reading bugmail)]

Setting b2g18-v1.0.1 --> affected until the Gaia changes land.  Please change this flag back once the Gaia changes land.

Comment 80

[John Ford [:jhford] CET/CEST Berlin Time]

this bug was not on v1-train.  correcting the flags

Comment 81

[Justin Lebar (not reading bugmail)]

I clearly have no idea how the Gaia flags are supposed to work, but we did land this on gecko b2g18 (comment 65), so shouldn't status-b2g18 be fixed?

Comment 82

[John Ford [:jhford] CET/CEST Berlin Time]

status-b2g18 is for the non-gecko parts of b2g, aiui.

dac3c41ee37d787a9483f41ea5c40a266f09ad59 is uplifted as:

v1-train: f282c74de639a6e81b555e89c249574555079aab
v1.0.1: 04f8dc51734e77bd8409a2dc44f06ce0ac073287

Comment 83

[amiller]

Does not make sense to create a regression issue.

Comment 84

[Cody Roesch [:croesch]]

Can you please provide steps to verify this fix - as we will blackbox test from the UI?

Comment 85

[Justin Lebar (not reading bugmail)]

Don't worry about it; I'm writing automated tests for this.

Comment 86

[Cody Roesch [:croesch]]

OK.
Adding QARegressExclude tag per comment 85.