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

RESOLVED FIXED in Firefox 21

Status

Firefox OS
General
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

(Blocks: 1 bug)

unspecified
B2G C4 (2jan on)
Dependency tree / graph
Bug Flags:
in-moztrap -

Firefox Tracking Flags

(blocking-b2g:tef+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

(Whiteboard: QARegressExclude)

Attachments

(12 attachments, 2 obsolete attachments)

839 bytes, patch
cjones
: review+
Details | Diff | Splinter Review
5.91 KB, patch
cjones
: review+
Details | Diff | Splinter Review
9.69 KB, patch
cjones
: review+
Details | Diff | Splinter Review
9.77 KB, patch
cjones
: review+
kanru
: feedback+
Details | Diff | Splinter Review
3.14 KB, patch
Details | Diff | Splinter Review
18.04 KB, patch
cjones
: review+
Details | Diff | Splinter Review
1.01 KB, patch
cjones
: review+
Details | Diff | Splinter Review
3.79 KB, patch
cjones
: review+
Details | Diff | Splinter Review
28.92 KB, patch
cjones
: review+
fabrice
: review+
Details | Diff | Splinter Review
5.44 KB, patch
cjones
: review+
Details | Diff | Splinter Review
19.73 KB, patch
cjones
: review+
Details | Diff | Splinter Review
355 bytes, text/html
vingtetun
: review+
etienne
: review+
Details
(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 836638
(Assignee)

Comment 1

5 years ago
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.)
Blocks blockers to tef+
blocking-b2g: --- → tef+
(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.
(Assignee)

Comment 4

5 years ago
> 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.
(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)
(Assignee)

Comment 6

5 years ago
> 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.
Who should be the assignee here?  jlebar?  clian?
Flags: needinfo?(justin.lebar+bug)
(Assignee)

Comment 8

5 years ago
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.
Assignee: nobody → justin.lebar+bug
Flags: needinfo?(justin.lebar+bug)
(Assignee)

Comment 9

5 years ago
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.
status-b2g18: --- → affected
status-b2g18-v1.0.1: --- → affected
(Assignee)

Comment 10

5 years ago
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.
(Assignee)

Comment 11

5 years ago
Created attachment 712771 [details] [diff] [review]
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.
Attachment #712771 - Flags: review?(jones.chris.g)
(Assignee)

Comment 12

5 years ago
Created attachment 712772 [details] [diff] [review]
Part 2: Add PROCESS_PRIORITY_FOREGROUND_HIGH.
Attachment #712772 - Flags: review?(jones.chris.g)
(Assignee)

Comment 13

5 years ago
Created attachment 712773 [details] [diff] [review]
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...
Attachment #712773 - Flags: review?(kchen)
Attachment #712773 - Flags: review?(jones.chris.g)
(Assignee)

Comment 14

5 years ago
Created attachment 712774 [details] [diff] [review]
Part 4: Add PowerManagerService::NewWakeLockOnBehalfOfProcess.
Attachment #712774 - Flags: review?(jones.chris.g)
(Assignee)

Comment 15

5 years ago
Created attachment 712775 [details] [diff] [review]
Part 5: Add nsIMozBrowserFrame::isExpectingSystemMessage.

I'm happy to change the attribute name; it's currently "expecting-system-message".
Attachment #712775 - Flags: review?(bzbarsky)
(Assignee)

Comment 16

5 years ago
Created attachment 712776 [details] [diff] [review]
Part 6: If a process is "critical" and holds the CPU wake lock, give it priority FOREGROUND_HIGH.
Attachment #712776 - Flags: review?(jones.chris.g)
(Assignee)

Comment 17

5 years ago
Created attachment 712777 [details] [diff] [review]
Part 7: Hold a CPU wake lock while a "critical" process that's expecting a system message loads.
Attachment #712777 - Flags: review?(jones.chris.g)
(Assignee)

Comment 18

5 years ago
Created attachment 712778 [details] [diff] [review]
Part 8: Inform the mozbrowser embedder when we're opening a frame that we expect to send a system message to.
Attachment #712778 - Flags: review?(jones.chris.g)
(Assignee)

Comment 19

5 years ago
Created attachment 712779 [details] [diff] [review]
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?
Attachment #712779 - Flags: review?(jones.chris.g)
(Assignee)

Comment 20

5 years ago
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.
(Assignee)

Updated

5 years ago
Summary: Hold processes in FOREGROUND when they have a pending system message → Hold processes in FOREGROUND or FOREGROUND_HIGH when they have a pending system message
(Assignee)

Comment 21

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

Missed a spot.
Attachment #712777 - Attachment is obsolete: true
Attachment #712777 - Flags: review?(jones.chris.g)
Attachment #712784 - Flags: review?(jones.chris.g)
(Assignee)

Comment 22

5 years ago
Created attachment 712786 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8053

Pointer to Github pull-request
(Assignee)

Updated

5 years ago
Attachment #712786 - Flags: review?(21)
Comment on attachment 712775 [details] [diff] [review]
Part 5: Add nsIMozBrowserFrame::isExpectingSystemMessage.

r=me, I guess.  Setting attributes for this feels kinda heavyweight.... :(
Attachment #712775 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 24

5 years ago
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.
(Assignee)

Comment 25

5 years ago
(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.
I'm working through a build issue (machine problem) that's preventing me from testing these patches; sorry.  Will review tomorrow morning my time.
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 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.)
Attachment #712771 - Flags: review?(jones.chris.g) → review+
Attachment #712772 - Flags: review?(jones.chris.g) → review+
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.
Attachment #712773 - Flags: review?(kchen)
Attachment #712773 - Flags: review?(jones.chris.g)
Attachment #712773 - Flags: review+
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
Attachment #712774 - Flags: review?(jones.chris.g)
Attachment #712774 - Flags: review+
Attachment #712774 - Flags: feedback?(kchen)
(Assignee)

Comment 31

5 years ago
(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 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.
Attachment #712776 - Flags: review?(jones.chris.g) → review+
(Assignee)

Updated

5 years ago
Attachment #712784 - Flags: review?(fabrice)
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.
Attachment #712784 - Flags: review?(jones.chris.g) → review+
Attachment #712778 - Flags: review?(jones.chris.g) → review+
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?
Attachment #712779 - Flags: review?(jones.chris.g) → review+
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.)
Attachment #712786 - Flags: review?(21) → review-
(Assignee)

Comment 36

5 years ago
>>+    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.
(Assignee)

Comment 37

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

Oops; I meant to do that, just forgot.
(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.
(Assignee)

Comment 39

5 years ago
> 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.
Attachment #712784 - Flags: review?(fabrice) → review+
(Assignee)

Comment 40

5 years ago
Created attachment 713462 [details] [diff] [review]
Part 0: Add constants CONTENT_PROCESS_{UNKNOWN_ID,MAIN_ID} to HalTypes.h and remove the equivalent constants from ContentParent.h.
Attachment #713462 - Flags: review?(jones.chris.g)
Attachment #713462 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 41

5 years ago
Created attachment 713747 [details] [diff] [review]
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.
Attachment #713747 - Flags: review?(jones.chris.g)
(Assignee)

Comment 42

5 years ago
Created attachment 713749 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8053

Pointer to Github pull-request
(Assignee)

Comment 43

5 years ago
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.
Attachment #713749 - Flags: review?(etienne)
Attachment #713749 - Flags: review?(21)
(Assignee)

Comment 44

5 years ago
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.  :)
(Assignee)

Comment 45

5 years ago
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 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 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.
Attachment #713747 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 48

5 years ago
> 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...
(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!
(Assignee)

Comment 50

5 years ago
> 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.
(Assignee)

Updated

5 years ago
Attachment #712786 - Attachment is obsolete: true
(Assignee)

Comment 52

5 years ago
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.
Keywords: qawanted
(Assignee)

Comment 53

5 years ago
And hopefully this is the only build issue I have.

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

Comment 54

5 years ago
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 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.
Attachment #713749 - Flags: review?(etienne) → review+
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.
Attachment #713749 - Flags: review?(21) → review+
(Assignee)

Comment 59

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

Updated

5 years ago
Depends on: 841563
(Assignee)

Updated

5 years ago
Depends on: 841634
(Assignee)

Comment 61

5 years ago
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).
(Assignee)

Updated

5 years ago
Whiteboard: [leave open]
(Assignee)

Comment 62

5 years ago
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.
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
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
status-b2g18: affected → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: affected → fixed
status-firefox19: --- → wontfix
status-firefox20: --- → wontfix
status-firefox21: --- → fixed
Target Milestone: --- → B2G C4 (2jan on)
Thanks, jelebar.  removing qawanted
Keywords: qawanted
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.
(Assignee)

Comment 68

5 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 69

5 years ago
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?
(Assignee)

Comment 70

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 841994
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
Attachment #712774 - Flags: feedback?(kchen) → feedback+
Merged in: https://github.com/mozilla-b2g/gaia/commit/dac3c41ee37d787a9483f41ea5c40a266f09ad59
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 73

5 years ago
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.
(Assignee)

Comment 74

5 years ago
(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?
(Assignee)

Updated

5 years ago
Depends on: 842679
(Assignee)

Comment 75

5 years ago
Vivien, can please you advise what we need to do to get this patch uplifted to the correct Gaia repositories?
Flags: needinfo?(21)
(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.
Flags: needinfo?(21) → needinfo?(akeybl)
(Assignee)

Comment 77

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

Updated

5 years ago
Depends on: 845001
(Assignee)

Comment 78

5 years ago
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.
(Assignee)

Comment 79

5 years ago
Setting b2g18-v1.0.1 --> affected until the Gaia changes land.  Please change this flag back once the Gaia changes land.
status-b2g18-v1.0.1: fixed → affected

Updated

5 years ago
Whiteboard: [Must land to v1.0.1 with bug 803688]
Whiteboard: [Must land to v1.0.1 with bug 803688]
this bug was not on v1-train.  correcting the flags
status-b2g18: fixed → affected
(Assignee)

Comment 81

5 years ago
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?
status-b2g18 is for the non-gecko parts of b2g, aiui.

dac3c41ee37d787a9483f41ea5c40a266f09ad59 is uplifted as:

v1-train: f282c74de639a6e81b555e89c249574555079aab
v1.0.1: 04f8dc51734e77bd8409a2dc44f06ce0ac073287
status-b2g18: affected → fixed
status-b2g18-v1.0.1: affected → fixed
(Assignee)

Updated

5 years ago
Flags: needinfo?(akeybl)

Comment 83

5 years ago
Does not make sense to create a regression issue.
Flags: in-moztrap-
Can you please provide steps to verify this fix - as we will blackbox test from the UI?
(Assignee)

Comment 85

5 years ago
Don't worry about it; I'm writing automated tests for this.
OK.
Adding QARegressExclude tag per comment 85.
Whiteboard: QARegressExclude
Depends on: 942955
Depends on: 1046033
You need to log in before you can comment on or make changes to this bug.