Closed Bug 860799 Opened 7 years ago Closed 7 years ago

Give frames expecting a system message priority above vanilla BACKGROUND

Categories

(Firefox OS Graveyard :: General, defect)

defect
Not set

Tracking

(blocking-b2g:tef+, b2g18 fixed, b2g18-v1.0.1 fixed)

RESOLVED FIXED
1.0.1 Cert2 (21may)
blocking-b2g tef+
Tracking Status
b2g18 --- fixed
b2g18-v1.0.1 --- fixed

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

Details

(Whiteboard: IOT, Spain [madrid])

Attachments

(2 files, 1 obsolete file)

Right now frames that are expecting system messages get vanilla BACKGROUND priority.  This means that those processes can be killed by the app you're currently using (FOREGROUND priority).  But it also means those processes can be killed by any other BACKGROUND process.  That's probably not what we want.

It would make sense to me to introduce a new priority between BACKGROUND and BACKGROUND_PERCEIVABLE, and give frames expecting system messages that new priority.  Maybe instead we give these frames BG_PERRCEIVABLE priority, but then I'm afraid that receiving an SMS will kill your music player; that may or may not be what we want.
See also the discussion in bug 859743 comment 11 and onwards.
Depends on: 844323
blocking-b2g: --- → tef+
Duplicate of this bug: 859743
Adding dependency to certification
Blocks: 855322
Daniel, could you help me understand what is the requirement here?

With our current architecture, we have to place the SMS app somewhere in an explicit hierarchy of processes.  If we place it too low in the hierarchy, the SMS app will get killed due to OOM.  If on the other hand we place it too high in the hierarchy, the SMS app will OOM other apps.

Do you guys consider receiving an SMS app to be a high-priority event, such that we should kill the foreground-most process if necessary?  Or, if we should never kill the foreground app, should we kill the music player to deliver your SMS?
Flags: needinfo?(dcoloma)
(In reply to Justin Lebar [:jlebar] from comment #4)
> Daniel, could you help me understand what is the requirement here?
> 
> With our current architecture, we have to place the SMS app somewhere in an
> explicit hierarchy of processes.  If we place it too low in the hierarchy,
> the SMS app will get killed due to OOM.  If on the other hand we place it
> too high in the hierarchy, the SMS app will OOM other apps.
> 
> Do you guys consider receiving an SMS app to be a high-priority event, such
> that we should kill the foreground-most process if necessary? 

No, I don't think we should do this.

> Or, if we should never kill the foreground app, should we kill the music player to
> deliver your SMS?

Yes, I think letting user know the SMS has arrived has higher priority than a music player in background
Flags: needinfo?(dcoloma)
Justin, over to you. If you don't have time to finish this work this week, please let me know.
Assignee: nobody → justin.lebar+bug
Whiteboard: IOT, Spain
Whiteboard: IOT, Spain → IOT, Spain [madrid]
Whiteboard: IOT, Spain [madrid] → IOT, Spain [madrid][status: needs more investigation]
Justin, do you think it's possible to solve it and close it before April 24th? It looks like we need some discussions and it does not look like a bug that we can fix before that date. Please tell us if you see something different. Thanks.
This is unlikely to be fixed by April 24.
Reviewed on April 23th: 
    Need to be fixed for certification. 
    Need QA to be involved in the testing. 
    Agreed to have this fixed before the 3rd certification.
marking qawanted for verification.
Keywords: qawanted
Based on what I'm reading on this bug, there's nothing to be tested here yet. When testing comes available and requires QA involvement, feel free to flag down again.
Keywords: qawanted
Attached patch TestSplinter Review
Attached patch Patch, v1 (obsolete) — Splinter Review
Attachment #741432 - Flags: review?(khuey)
Attachment #741431 - Attachment description: Test that we give frames expecting a system message priority above vanilla BACKGROUND. → Test
Attachment #741431 - Flags: review?(khuey)
Attached patch Patch, v2Splinter Review
Now with less enable logging.
Attachment #741432 - Attachment is obsolete: true
Attachment #741432 - Flags: review?(khuey)
Attachment #741434 - Flags: review?(khuey)
This isn't totally right, in the following way:

What we want is that if frame X is not visible, frame X has expecting-system-message, and frame X has acquired the CPU or high-priority wake lock, then the process containing frame X gets priority BACKGROUND_PERCEIVABLE.

But what we actually do is, if process Y has no frames that are visible, Y has at least one frame with expecting-system-message, and Y has acquired either of the wake locks, then Y gets priority BACKGROUND_PERCEIVABLE.

The problem is that we don't tie a wake lock to a frame.  This is actually a hard thing to fix, because we have this notion of acquiring a wake lock on behalf of a process /before any frames have been created/, and that's what we use when a frame is expecting a system message.

Anyway I don't think this is a big deal.  The worst thing that happens is that a frame gets bumped up to BG_PERCEIVABLE, and a frame can already do that by playing a silent ogg.
Whiteboard: IOT, Spain [madrid][status: needs more investigation] → IOT, Spain [madrid][status: needs review, needs dependencies landed]
Target Milestone: --- → 1.0.1 Cert2 (28may)
This needs a Gaia patch too.  I've had bad experiences trying to patch Gecko and Gaia in the same bug, so I'll do that separately.
Blocks: 865429
The OEM will need around 5 days to prepare the build and do the verification. It means, we need to fix it by May 21th. Justin, do you think it's possible? Thanks.
(In reply to Justin Lebar [:jlebar] from comment #16)
> This needs a Gaia patch too.  I've had bad experiences trying to patch Gecko
> and Gaia in the same bug, so I'll do that separately.

Thanks! In this case, would you mind creating another bug for Gaia patch?
(In reply to khu from comment #18)
> Thanks! In this case, would you mind creating another bug for Gaia patch?

 Justin Lebar [:jlebar] 9 hours ago
Blocks: 865429
https://hg.mozilla.org/mozilla-central/rev/911d21581db4
https://hg.mozilla.org/mozilla-central/rev/1f675d6e1783
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: IOT, Spain [madrid][status: needs review, needs dependencies landed] → IOT, Spain [madrid]
You need to log in before you can comment on or make changes to this bug.