844323 - Move process priority manager into the main process

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Justin Lebar (not reading bugmail)]

Right now each process gets its own process priority manager.

This is causing myriad problems, which I've addressed both by carefully fixing race conditions and by adding timeouts where the races are too hard.

I think we should just move this logic into the main process; there's no reason any longer for it to be in every content process.

Comment 1

[Justin Lebar (not reading bugmail)]

As part of this work, we can probably change the current behavior of the process priority manager (hereafter PPM) with respect to timeouts.

Right now we apply a timeout when going "down" in priority, but not one when coming "up" in priority.  Except moving between different background priorities isn't considered moving "down".  And also, if Gaia does setVisible(), the effects of that are felt immediately.

This is pretty weird behavior.  I'd like to get rid of the timeouts altogether, but if not, we can probably make it so that all "ups" happen immediately and all "downs" happen after a timeout.  Or something like that.

Comment 2

[Justin Lebar (not reading bugmail)]

I split out the timeout consistency issue into bug 844970.

Comment 3

[Justin Lebar (not reading bugmail)]

Blocks a blocker.  I don't see how to fix bug 847592 without this.

Comment 4

[Andreas Gal :gal]

Justin, who is working on this?

Comment 5

[Justin Lebar (not reading bugmail)]

I am.

Comment 6

[Lucas Adamski [:ladamski]]

Since you're PTO this week, is this something you can get done early next week or should we find another owner?

Comment 7

[Justin Lebar (not reading bugmail)]

Only cjones and I know this code, and this is a relatively large and highly risky change.  Please feel free to unleash someone on this if you'd like, but I doubt that doing so would move up the date by which this work would be completed significantly, after accounting for reviews and possible regressions.

We've been working under "get this done early next week" deadlines for months now, and that's what got us into this mess in the first place.  I'll get this done as quickly as I can, but it's not going to be landed early next week.  Friday of next week is probably the earliest I'd expect to see anything in the tree.

Comment 8

[Lucas Adamski [:ladamski]]

Thanks for the update, appreciate the honest estimate. :)

Comment 9

[Joe Cheng [:jcheng] (please needinfo)]

since bug 852847 is resolved, wonder what's the latest with this bug? thanks

Comment 10

[Justin Lebar (not reading bugmail)]

Because this is a risky change, I've been working on unit tests for this as I finish up the implementation.  I don't want to regress something, and if I do, I want to be able to write an automated test to ensure we don't re-regress.  The tests and reviews are taking most of my time here.

Comment 11

[Justin Lebar (not reading bugmail)]

https://tbpl.mozilla.org/?tree=Try&rev=7701c43599d7

Comment 12

[Justin Lebar (not reading bugmail)]

Attachment: Prelude part 1: Use a pref to in nsHTMLMediaElement to control whether we talk to the audio service.

Previously, we used #ifdef B2G.  Using a pref allows us to write mochitests which run in desktop Firefox that test the audio service and its interaction with other components.

Comment 13

[Justin Lebar (not reading bugmail)]

Attachment: Prelude part 2: Switch xpcom/base/StaticPtr from NULL to nullptr, and add a necessary include.

This is necessary to make this header compile in one of the places I use it.  Anyway it's the right change.

Comment 14

[Justin Lebar (not reading bugmail)]

Attachment: Prelude part 3: Change ProcessPriorityManager's export directory.

It was mozilla/dom/ipc/ before, but this changes it to mozilla/, which matches the class's namespace.

Comment 15

[Justin Lebar (not reading bugmail)]

Attachment: Part 1: Move process preallocation logic out of ContentParent and into a new file, PreallocatedProcessManager.

Also make the PreallocatedProcessManager respond to pref changes.  This allows us to write a test involving the preallocated process.  Making this change was the main motivation for this patch; I moved the logic out of ContentParent because with the pref-watching code it was becoming unweildy.

Ben, if you don't want to be the new co-owner of dom::ipc, let's find someone who does...

Comment 16

[Justin Lebar (not reading bugmail)]

Attachment: Part 2 (The Main Event): Move ProcesPriorityManager to the main process.

Comment 17

[Justin Lebar (not reading bugmail)]

Attachment: Part 3: ProcessPriorityManager tests.

These tests are great; they've already found a bunch of bugs in other pieces of code (those bugs are marked as blocking this bug).  Let's just hope the new tests go orange less-frequently than the current browser-element tests.

Nikhil, can you check that I'm using promises in a sane way here, in general?  No need to review everything carefully, unless you want to.

Ben, I expect more of an rs than a full review here.

Comment 18

[Justin Lebar (not reading bugmail)]

Comment on attachment 735588 [details] [diff] [review]
Part 2 (The Main Event): Move ProcesPriorityManager to the main process.

The ProcessPriorityManager* changes are a complete rewrite.  It may be easier to read the new file than to try to understand the diff.

You can view the file on github (or even check it out locally), if you want.

This branch contains changes for this bug and also bug 847592.

https://github.com/jlebar/mozilla-central/commits/process-priority-main-process

Pick one of the earlier commits listed there to see just the changes in this bug.  If it's not self-explanatory, just find me on IRC.

Comment 19

[Justin Lebar (not reading bugmail)]

Comment on attachment 735589 [details] [diff] [review]
Part 3: ProcessPriorityManager tests.

bjacob, can you please check the webgl pref setting at the bottom of test_WebGLContextLost?  We talked about this a few days ago on IRC, but I still think it's pretty scary, so I'd like to make sure that we understood each other properly.

Comment 20

[Andrea Marchesini [:baku]]

Comment on attachment 735584 [details] [diff] [review]
Prelude part 1: Use a pref to in nsHTMLMediaElement to control whether we talk to the audio service.

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

looks good!

Comment 21

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 735589 [details] [diff] [review]
Part 3: ProcessPriorityManager tests.

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

r- for now. Code looks good, but I'd like to clarification of

1) expectOnlyOneProcessCreated() and,
2) returning Promise.resolve() from success handlers of promises.

::: dom/browser-element/mochitest/browserElementTestHelpers.js
@@ +116,5 @@
> +    function(subject, topic, data) {
> +      if (observed) {
> +        return;
> +      }
> +      observed = true;

what is observed used for? If you're using it only to check that the promise isn't resolved twice, that's not required. If a promise has been resolved once, handlers waiting on that promise from before the promise was resolved won't trigger again.

var def = Promise.defer();
def.promise.then(function() { console.log("1"); })

def.resolve(5); // output: 1
// later if you add a handler, it is called immediately
def.promise.then(function() { console.log("2"); }) // output: 2

def.resolve(10); // does nothing

If observed is present for some other reason, can you explain in a comment?

@@ +136,5 @@
> +    expectProcessCreated().then(function(childID) {
> +      ok(false, 'Got unexpected process creation, childID=' + childID);
> +    });
> +  });
> +  return p;

Just to be clear, you are returning the same promise that the first call to expectProcessCreated() returns. Which means if another process does get created, only the test fails, but functions waiting on the promise returned from expectProcessCreated() still succeed. If that's what you want, then great :)

@@ +149,5 @@
> +  var observed = false;
> +  browserElementTestHelpers.addProcessPriorityObserver(
> +    'process-priority-set',
> +    function(subject, topic, data) {
> +      if (observed) {

same as above

::: dom/browser-element/mochitest/priority/test_Audio.html
@@ +23,5 @@
> +  iframe.setAttribute('mozbrowser', true);
> +  iframe.src = 'file_Audio.html';
> +
> +  var childID = null;
> +  expectOnlyOneProcessCreated().then(function(chid) {

This is where the promise will resolve as soon as the first process is created, and not when 'only one process is created'

::: dom/browser-element/mochitest/priority/test_ExpectingSystemMessage.html
@@ +22,5 @@
> +
> +var iframe = null;
> +var childID = null;
> +
> +function expectPriorityChange(expectedPriority, callback)

could you comment why you aren't using the promises version?

::: dom/browser-element/mochitest/priority/test_WebGLContextLost.html
@@ +31,5 @@
> +      [expectPriorityChange(childID, 'FOREGROUND'),
> +       expectMozbrowserEvent(iframe, 'loadend'),
> +       expectMozbrowserEvent(iframe, 'showmodalprompt').then(function(e) {
> +         is(e.detail.message, 'ready');
> +         return Promise.resolve();

Is their a reason that you're using a pre-resolved promise here rather than just letting the function end since then() itself returns a promise.

@@ +52,5 @@
> +    // WebGL context before we fire the low-memory notification ourself.
> +
> +    var p = expectMozbrowserEvent(iframe, 'showmodalprompt').then(function(e) {
> +      is(e.detail.message, 'webglcontextlost');
> +      return Promise.resolve();

here too.

Comment 22

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 735589 [details] [diff] [review]
Part 3: ProcessPriorityManager tests.

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

That's not scary. Ideally our WebGL blacklisting would work in child processes. When I did it, we only did any Web rendering in the main process. So it sucks a bit that you have to force-enable in child processes. But it's not scary to do that in a unit test--- in the worst case, someone running this on a machine that can't actually do WebGL would get consistent test failures, but as virtually all machines can do WebGL, that's not a big deal. I don't have security concerns either because nobody uses a mochitest browser instance to do actual browsing.

Comment 23

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 735587 [details] [diff] [review]
Part 1: Move process preallocation logic out of ContentParent and into a new file, PreallocatedProcessManager.

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

r=me with these addressed.

::: dom/ipc/ContentParent.h
@@ +87,5 @@
>  
>      /**
> +     * Create a subprocess suitable for use as a preallocated app process.
> +     */
> +    static already_AddRefed<ContentParent> PreallocateAppProcess();

Since this is only supposed to be called by PreallocatedProcessManager would it make sense to make PreallocatedProcessManager a friend and hide this in a private section?

@@ +159,5 @@
>  private:
>      static nsDataHashtable<nsStringHashKey, ContentParent*> *gAppContentParents;
>      static nsTArray<ContentParent*>* gNonAppContentParents;
>      static nsTArray<ContentParent*>* gPrivateContent;
> +    static LinkedList<ContentParent> gContentParents;

Nit: This is a static member, 's' is the better prefix :(

::: dom/ipc/PreallocatedProcessManager.cpp
@@ +17,5 @@
> +/**
> + * This singleton class implements the static methods on
> + * PreallocatedProcessManager.
> + */
> +class PreallocatedProcessManagerImpl MOZ_FINAL

I would put this in an anonymous namespace.

@@ +75,5 @@
> +  return PreallocatedProcessManagerImpl::Singleton()->Take();
> +}
> +
> +/* static */ StaticRefPtr<PreallocatedProcessManagerImpl>
> +  PreallocatedProcessManagerImpl::sSingleton;

Nit: Spaces here, and you don't really need the /*static*/ here, it's obvious.

@@ +96,5 @@
> +  : mEnabled(false)
> +{}
> +
> +void
> +PreallocatedProcessManagerImpl::Init()

In general if we have an Init method that can't fail I would prefer it just get inlined into the constructor. Init methods are only useful imo if they can fail.

@@ +115,5 @@
> +{
> +  if (!strcmp("ipc:content-shutdown", aTopic)) {
> +    ObserveProcessShutdown(aSubject);
> +  } else {
> +    // The only other observer we registered was for our prefs.

I always assert this sort of thing.

::: dom/ipc/PreallocatedProcessManager.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#pragma once

This needs real include guards, I don't think we have full support for this yet. (khuey confirms)

@@ +42,5 @@
> +   * Create a process after a delay.  We wait for a period of time (specified
> +   * by the dom.ipc.processPrelaunch.delayMs pref), then wait for this process
> +   * to go idle, then allocate the new process.
> +   *
> +   * If mEnabled is false, or if we already have a preallocated process, this

Nit: There's no mEnabled here, probably want to say "if the preference is set to disabled". Below too.

@@ +74,5 @@
> +   * After you Take() the preallocated process, you need to call one of the
> +   * Allocate* functions (or change the dom.ipc.processPrelaunch pref from
> +   * false to true) before we'll create a new process.
> +   */
> +  static already_AddRefed<dom::ContentParent> Take();

Nit: It would be nicer if you |typedef mozilla::dom::ContentParent ContentParent| above.

Comment 24

[Justin Lebar (not reading bugmail)]

@@ +159,5 @@
>>  private:
>>      static nsDataHashtable<nsStringHashKey, ContentParent*> *gAppContentParents;
>>      static nsTArray<ContentParent*>* gNonAppContentParents;
>>      static nsTArray<ContentParent*>* gPrivateContent;
>> +    static LinkedList<ContentParent> gContentParents;
>
> Nit: This is a static member, 's' is the better prefix :(

Our number-one rule is to match the existing style of a file, so I'd rather not
do something different than the rest of the file.  If you don't like the fact
that I'm adding another 'g' prefixed variable, how about I change the whole
file from g to s?

> Nit: Spaces here, and you don't really need the /*static*/ here, it's obvious.

Do you mind if I keep the /* static */?  Note that it's a major error to use
StaticRefPtr in a non-static context and we can't detect this automatically, so
I prefer being explicit about it.

> In general if we have an Init method that can't fail I would prefer it just
> get inlined into the constructor. Init methods are only useful imo if they
> can fail.

It's an error to call XPCOM methods from a constructor, since mRefcnt==0, so
this has to be in a separate Init().

> I always assert this sort of thing.

I do too, except I was too lazy to look up the topic for prefs.  :)  I'll
change it.

> This needs real include guards, I don't think we have full support for this
> yet. (khuey confirms)

Kyle, what's the actual rule on this?  We use pragma once in a number of
places, and the webidl generator even spits it out in its suggested header
implementations...

> Nit: It would be nicer if you |typedef mozilla::dom::ContentParent ContentParent| above.

I don't feel strongly about it, but I think that using |typedef| as a
replacement for |using| inside header files is an anti-pattern; typedef is not
a substitute for |using|.

At the very least, we'll get worse compile errors with a typedef.  I can't
think of any other harms...

Comment 25

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Justin Lebar [:jlebar] from comment #24)
> Our number-one rule is to match the existing style of a file, so I'd rather
> not
> do something different than the rest of the file.  If you don't like the fact
> that I'm adding another 'g' prefixed variable, how about I change the whole
> file from g to s?

If you don't mind that would indeed be better.

> Do you mind if I keep the /* static */?

Sure.

> It's an error to call XPCOM methods from a constructor, since mRefcnt==0, so
> this has to be in a separate Init().

Yep, you're right. I was just focused on the void return and forgot about the refcount stuff.

> I don't feel strongly about it, but I think that using |typedef| as a
> replacement for |using| inside header files is an anti-pattern; typedef is
> not
> a substitute for |using|.

Sorry, I meant inside the class, not at global level or inside the namespace.

Comment 26

[Justin Lebar (not reading bugmail)]

I think there's a bug around nested mozbrowsers.  If I have

  <iframe mozbrowser id=A> <-- in-process
    <iframe mozbrowser id=B> <-- oop

and I setVisible(false) on A, it should send B to the background.  I don't think my code does this atm.  I'll attach a fix as a separate patch to fold in on checkin, and also add a test.

Comment 27

[Justin Lebar (not reading bugmail)]

Attachment: Part 3, v2: ProcessPriorityManager tests.

Comment 28

[Justin Lebar (not reading bugmail)]

Thanks for the review!

>1) expectOnlyOneProcessCreated() and,
>2) returning Promise.resolve() from success handlers of promises.

>::: dom/browser-element/mochitest/browserElementTestHelpers.js
>@@ +116,5 @@
>> +    function(subject, topic, data) {
>> +      if (observed) {
>> +        return;
>> +      }
>> +      observed = true;
>
> What is observed used for? If you're using it only to check that the promise
> isn't resolved twice, that's not required.

I was indeed guarding against resolving the promise twice, but also I don't
want to do ok(true) twice, and we need the guard for that.  I added a comment.

>@@ +136,5 @@
>> +    expectProcessCreated().then(function(childID) {
>> +      ok(false, 'Got unexpected process creation, childID=' + childID);
>> +    });
>> +  });
>> +  return p;

>Just to be clear, you are returning the same promise that the first call to
>expectProcessCreated() returns. Which means if another process does get
>created, only the test fails, but functions waiting on the promise returned
>from expectProcessCreated() still succeed. If that's what you want, then great
>:)

Right.  Anyway there's no way to do something else, right?  I want to resolve
right away when the process is created, and I'd have to wait a while if I
wanted to reject on the occasion of a second process being created.  Anyway the
second process might be created as a /result/ of resolving this promise!

>@@ +149,5 @@
>> +  var observed = false;
>> +  browserElementTestHelpers.addProcessPriorityObserver(
>> +    'process-priority-set',
>> +    function(subject, topic, data) {
>> +      if (observed) {

> same as above

This one definitely requires |observed|; otherwise, the |is| calls will fail on
the next priority change.

>::: dom/browser-element/mochitest/priority/test_Audio.html
>@@ +23,5 @@
>> +  iframe.setAttribute('mozbrowser', true);
>> +  iframe.src = 'file_Audio.html';
>> +
>> +  var childID = null;
>> +  expectOnlyOneProcessCreated().then(function(chid) {

>This is where the promise will resolve as soon as the first process is
>created, and not when 'only one process is created'

Right; it's impossible to have a promise that resolves when 'only one process
is created during the test' if the test can't complete until this promise
resolves.  :)

>::: dom/browser-element/mochitest/priority/test_ExpectingSystemMessage.html
>@@ +22,5 @@
>> +
>> +var iframe = null;
>> +var childID = null;
>> +
>> +function expectPriorityChange(expectedPriority, callback)

>could you comment why you aren't using the promises version?

I version of this one which uses promises, but it was in the wrong patch.  I've
fixed that here.  Thanks for noticing!

>::: dom/browser-element/mochitest/priority/test_WebGLContextLost.html
>@@ +31,5 @@
>> +      [expectPriorityChange(childID, 'FOREGROUND'),
>> +       expectMozbrowserEvent(iframe, 'loadend'),
>> +       expectMozbrowserEvent(iframe, 'showmodalprompt').then(function(e) {
>> +         is(e.detail.message, 'ready');
>> +         return Promise.resolve();

>Is their a reason that you're using a pre-resolved promise here rather than
>just letting the function end since then() itself returns a promise.

No, I just didn't know that worked.

Comment 29

[Justin Lebar (not reading bugmail)]

Bug 860526 comment 6 is suggesting that we cannot in fact renice threads from outside the process which owns the threads.

That's a bummer, but I don't think we should put the breaks on this patch.  Renicing other threads in the child process didn't work before this patch, either.  We may have to send a message down to the child process to get it to renice its non-main threads, but at least we can renice its main thread immediately.

Perhaps more importantly, moving the process priority manager into the main thread lets us set the child processes' oom_adj's immediately.  The worst thing that happens when we have a delay in setting nice values is that the phone runs slowly, but the worst thing that happens when we have a delay in setting oom_adj values is that apps crash.

Comment 30

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 736346 [details] [diff] [review]
Part 3, v2: ProcessPriorityManager tests.

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

r=me with 2 changes.

::: dom/browser-element/mochitest/priority/test_Background.html
@@ +42,5 @@
> +    return p;
> +  }).then(function() {
> +    var p = expectPriorityChange(childID, 'FOREGROUND');
> +    iframe.setVisible(true);
> +  }).then(SimpleTest.finish);

Here, if you don't return p, SimpleTest.finish() is called before expectPriorityChange(childID, 'FOREGROUND') happens.

then() return's a promise whose resolution is decided by it's onFulfill handler (first argument).
If the onFulfill return a value, the promise returned by then() resolves with the value.
If the onFulfill throws an error, the promise returned by then() is rejected.
If the onFulfill returns a `promise`, the promise returned by then() is driven by the promise returned by onFulfill.

e.g.:
var p1, p2 = Promise.defer(), Promise.defer();
p1.promise.then(function() { console.log("Step 1"); return p2.promise; /** KEY **/ }).then(function() { console.log("Step 2"); })
p1.resolve(); // Step 1
p2.resolve(); // Step 2

::: dom/browser-element/mochitest/priority/test_HighPriority.html
@@ +64,5 @@
> +      alertTimes.push(new Date());
> +
> +      // Block the alert; we'll unblock it by calling e.detail.unblock() later.
> +      e.preventDefault();
> +      return Promise.resolve(e.detail.unblock);

return e.detail.unblock;

Comment 31

[Justin Lebar (not reading bugmail)]

> Bug 860526 comment 6 is suggesting that we cannot in fact renice threads from outside the process 
> which owns the threads.

Looks like this is wrong; you can renice a tid just like you renice a pid.  That's a good thing; it means we can leave all our logic in the parent process; we just need to change setpriority() to nice(), so we make a relative priority change to the thread's priority instead of setting an absolute priority.

Comment 32

[Justin Lebar (not reading bugmail)]

Note to self: Because of bug 862414, we need to disable test_Preload on Mac (and we should disable it on Windows too, for good measure).

Comment 33

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 735588 [details] [diff] [review]
Part 2 (The Main Event): Move ProcesPriorityManager to the main process.

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

::: content/base/src/nsFrameLoader.cpp
@@ +2571,5 @@
> +  mVisible = aVisible;
> +  nsCOMPtr<nsIObserverService> os = services::GetObserverService();
> +  if (os) {
> +    os->NotifyObservers(NS_ISUPPORTS_CAST(nsIFrameLoader*, this),
> +                        "frameloader-visible-changed", nullptr);

Do you want to bail out and not notify if the visibility has not actually changed? (E.g. setting visible to true twice will trigger two notifications currently.)

::: dom/ipc/ContentChild.cpp
@@ +1185,5 @@
> +    props->SetPropertyAsInt32(NS_LITERAL_STRING("priority"),
> +                              static_cast<int32_t>(aPriority));
> +
> +    os->NotifyObservers(static_cast<nsIPropertyBag2*>(props),
> +                        "ipc:process-priority-changed",  nullptr);

Nit: extra space after comma

::: dom/ipc/ContentParent.cpp
@@ +473,5 @@
>  
>      nsRefPtr<TabParent> tp = new TabParent(aContext);
>      tp->SetOwnerElement(aFrameElement);
>      PBrowserParent* browser = p->SendPBrowserConstructor(
> +        nsRefPtr<TabParent>(tp).forget().get(), // DeallocPBrowserParent() releases this ref.

This hurts my head a little... Is it necessary?

@@ +680,5 @@
> +    nsCOMPtr<nsIAppsService> appsService = do_GetService(APPS_SERVICE_CONTRACTID);
> +    NS_ENSURE_TRUE_VOID(appsService);
> +
> +    nsCOMPtr<mozIDOMApplication> domApp;
> +    appsService->GetAppByManifestURL(aManifestURL, getter_AddRefs(domApp));

Shouldn't this warn if it fails?

::: dom/ipc/ContentParent.h
@@ +138,5 @@
>          return mSubprocess;
>      }
>  
> +    int32_t Pid() {
> +        return base::GetProcId(mSubprocess->GetChildProcessHandle());

Hrm. On windows base::GetProcId returns a DWORD (unsigned). Can we just make this return base::ProcessId?

::: dom/ipc/ProcessPriorityManager.cpp
@@ +36,2 @@
>  //
> +// (Wow, our logging story is a huge mess.)

I can't believe we don't have something more generic that does all these gymnastics by now. Nothing in mfbt?

::: dom/ipc/ProcessPriorityManager.h
@@ +9,5 @@
>  
> +#include "mozilla/HalTypes.h"
> +#include "mozilla/StaticPtr.h"
> +#include "nsIObserver.h"
> +#include "nsDataHashtable.h"

Nit: It looks like only the first include is needed here.

::: dom/ipc/TabParent.cpp
@@ +222,5 @@
>  
>  void
> +TabParent::GetAppType(nsAString& aOut)
> +{
> +  aOut.Truncate();

Nit: I'd just put this inside the |elem| check to only modify the string once.

Comment 34

[Justin Lebar (not reading bugmail)]

Attachment: Part 2a: Make the in-process ProcessPriorityManager handle <iframe mozbrowser> inside <iframe mozbrowser> correctly.

This is kind of ugly, but :shrug:.

I'll fold this into part 2.

Comment 35

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 735588 [details] [diff] [review]
Part 2 (The Main Event): Move ProcesPriorityManager to the main process.

> namespace mozilla {

I would declare the classes in an anonymous namespace, then switch to mozilla later if you want.

>    * given topic with the given data.  This is used for testing

Nit: End with a .

>   ProcessPriorityManagerImpl();

You should add an empty (virtual, if you care) destructor here to prevent the implicit one from being publicly accessible. For ProcessPriorityManagerChild too.

>   int32_t Pid();
>   uint64_t ChildID();

These could both be const methods.

>   const char* NameWithComma();

Why not just return a |const nsAutoCString&| and return mNameWithComma? Raw char pointers that have funny lifetimes shouldn't be needed in this decade imho :)

> ProcessPriorityManager::SetProcessPriority(ContentParent* aContentParent,
>                                            ProcessPriority aPriority)

I would assert aContentParent here.

>   // The process priority manager is main-process only.
>   if (XRE_GetProcessType() != GeckoProcessType_Default) {
>     return;

You could set sInitialized here to avoid further calls to this function.

> ProcessPriorityManagerImpl::Init()
> {
>   // The process priority manager is main-process only.  We should have checked
>   // that before calling this function!
>   NS_ENSURE_TRUE_VOID(XRE_GetProcessType() == GeckoProcessType_Default);

Just MOZ_ASSERT this instead.

>   hal::SetProcessPriority(getpid(), PROCESS_PRIORITY_MASTER);
> 
>   nsCOMPtr<nsIObserverService> obs = services::GetObserverService();

This will fail after we begin the shutdown process, I would probably guard here.

> ProcessPriorityManagerImpl::Observe(
>   nsISupports* aSubject,
>   const char* aTopic,
>   const PRUnichar* aData)
> {
>   nsAutoCString topic(aTopic);

This should be nsDependentCString to avoid copying.

>   if (topic == NS_LITERAL_CSTRING("ipc:content-created")) {

topic.EqualsLiteral("..."), below too.

>     nsCOMPtr<nsIObserver> cp = do_QueryInterface(aSubject);

Why is this extra ref necessary? If it really is then why isn't there a ref for the destroyed case? Can you just pass aSubject to ObserveContentParentCreated like you do for ObserveContentParentDestroyed?

> ProcessPriorityManagerImpl::SetProcessPriority(ContentParent* aContentParent,
>                                                ProcessPriority aPriority)

I'd assert aContentParent

>   nsRefPtr<ParticularProcessPriorityManager> pppm;
>   mParticularManagers.Get(childID, &pppm);
>   MOZ_ASSERT(pppm);
>   if (pppm) {

I don't think you need to check here, your assertion should be correct.

> ParticularProcessPriorityManager::ParticularProcessPriorityManager(

You could assert that you're running in the main process here. I't implied by having a valid ContentParent, but it helps code readability too. Actually, you could add such an assertion to lots of methods here...

> ParticularProcessPriorityManager::Init()
> ...
>   nsCOMPtr<nsIObserverService> os = services::GetObserverService();

I'm not sure if you need to guard this one... Creating a child process probably shouldn't happen after we begin shutdown, but I wouldn't bet too much on it...

> ParticularProcessPriorityManager::Notify(const WakeLockInformation& aInfo)
> {
>   bool* dest = nullptr;
>   if (aInfo.topic() == NS_LITERAL_STRING("cpu")) {

EqualsLiteral, here and below.

> ParticularProcessPriorityManager::Observe(nsISupports* aSubject,
>                                           const char* aTopic,
>                                           const PRUnichar* aData)
> ...
>   nsAutoCString topic(aTopic);

nsDependentCString, and EqualsLiteral.

> ParticularProcessPriorityManager::OnRemoteBrowserFrameShown(nsISupports* aSubject)
> ...
>   nsCOMPtr<nsITabParent> tp;
>   fl->GetTabParent(getter_AddRefs(tp));
>   NS_ENSURE_TRUE_VOID(tp);
> 
>   nsRefPtr<TabParent> tabParent = static_cast<TabParent*>(tp.get());
>   if (tabParent->Manager() != mContentParent) {

The extra ref here is unnecessary, just TabParent* should be fine. Same thing in the next two methods.

>   uint32_t timeout = Preferences::GetUint(
>     nsPrintfCString("dom.ipc.processPriorityManager.%s", aTimeoutPref).get());

Appending is probably faster than running through printf, why not just do that?

> ParticularProcessPriorityManager::ComputePriority()

In this function you (maybe) get and iterate ManagedPBrowserParent three times... Couldn't you just do it once and look for all the relevant info, then make your priority decision? Then you could get rid of HasAppType...

> ParticularProcessPriorityManager::SetPriorityNow(ProcessPriority aPriority)

Can you assert that mContentParent is non-null here?

>   if (aPriority == PROCESS_PRIORITY_UNKNOWN) {
>     MOZ_ASSERT(false);
>     return;
>   }

If this isn't possible then just assert != PROCESS_PRIORITY_UNKNOWN, no runtime check.

> ParticularProcessPriorityManager::ShutDown()

I'd assert mContentParent here, just to make sure we never call this twice or something.

> ParticularProcessPriorityManager::FireTestOnlyObserverNotification(
> ...
>   if (!aData.IsEmpty()) {
>     data.AppendASCII(":");

AppendLiteral.

> ProcessPriorityManagerChild::Init()
> {
>   // The process priority should only be change child processes; don't even

Nit: s/change/changed/

>   int32_t priority = static_cast<int32_t>(PROCESS_PRIORITY_UNKNOWN);
>   props->GetPropertyAsInt32(NS_LITERAL_STRING("priority"), &priority);
>   NS_ENSURE_TRUE(priority != PROCESS_PRIORITY_UNKNOWN, NS_OK);

Shouldn't this need a cast too?

Comment 36

[Justin Lebar (not reading bugmail)]

Attachment: Part 3a: Test that we handle <iframe mozbrowser> in <iframe mozbrowser> properly with the new, in-process ProcessPriorityManager.

Comment 37

[Justin Lebar (not reading bugmail)]

> Appending is probably faster than running through printf, why not just do that?

Here and elsewhere: Because this code isn't performance-sensitive.  :)

Comment 38

[Justin Lebar (not reading bugmail)]

Ben, I'm being told that we want this landed by Wednesday, so a fast review would be much appreciated here.

Comment 39

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 735588 [details] [diff] [review]
Part 2 (The Main Event): Move ProcesPriorityManager to the main process.

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

Not sure whether I was just ridiculously tired or what but the things I pointed out above can be fixed before landing without needing to be re-reviewed. r=me.

Comment 40

[Justin Lebar (not reading bugmail)]

Attachment: Part 2a, v2: Make the in-process ProcessPriorityManager handle <iframe mozbrowser> inside <iframe mozbrowser> correctly.

Fixing a typo.  This patch never could have worked with the existing typo, so I must have attached an old version or something.

Comment 41

[Justin Lebar (not reading bugmail)]

Attachment: Part 2a, v3: Make the in-process ProcessPriorityManager handle <iframe mozbrowser> inside <iframe mozbrowser> correctly.

Now with less debugging printfs.

Comment 42

[Justin Lebar (not reading bugmail)]

https://tbpl.mozilla.org/?tree=Try&rev=b2cc3dbeb237

This needs to land atop bug 861441, which, in our current approach, needs bug 861434.  So we're not ready to land this just yet.

Comment 43

[Justin Lebar (not reading bugmail)]

>>   nsRefPtr<ParticularProcessPriorityManager> pppm;
>>   mParticularManagers.Get(childID, &pppm);
>>   MOZ_ASSERT(pppm);
>>   if (pppm) {
>
>I don't think you need to check here, your assertion should be correct.

I left this in because I'm not in control of this invariant; anyone (e.g. a
bogus extension) could fire this notification twice.

>> ParticularProcessPriorityManager::ParticularProcessPriorityManager(
>
>You could assert that you're running in the main process here. I't implied by having a valid ContentParent, but it helps code readability too. Actually, you could add such an assertion to lots of methods here...

I modified the class comments to be more explicit about which process they must
run in.  I don't want to put assertions everywhere, but I also put them on the
constructors and public statid methods.

>> ParticularProcessPriorityManager::Init()
>> ...
>>   nsCOMPtr<nsIObserverService> os = services::GetObserverService();
>
>I'm not sure if you need to guard this one... Creating a child process probably shouldn't happen after we begin shutdown, but I wouldn't bet too much on it...

Initializing the ProcessPriorityManager happens earlier, so it definitely
shouldn't happen after shutdown, but you asked me to put a guard there.  So I
just put a guard around all of the GetObserverService calls.

>>   uint32_t timeout = Preferences::GetUint(
>>     nsPrintfCString("dom.ipc.processPriorityManager.%s", aTimeoutPref).get());
>
>Appending is probably faster than running through printf, why not just do that?

Speed is not a concern here, as far as I'm aware.

If you have a cleaner way of doing this, I'm certainly all ears.  I don't feel like

  (NS_LITERAL_STRING("...") + nsDependentCString(aTimeoutPref)).get()

is an improvement, but you may have a more clever idea.

Note that I use this GetPref(nsPrintfCString(...).get()) idiom in a lot of code
here and in hal, and it's not always to do a simple append.  It's nice to have
one way of doing it...

>> ParticularProcessPriorityManager::ComputePriority()
>
>In this function you (maybe) get and iterate ManagedPBrowserParent three
>times... Couldn't you just do it once and look for all the relevant info, then
>make your priority decision? Then you could get rid of HasAppType...

I don't care about the speed issue here one bit.  But in terms of cleanliness,
I agree it's weird to iterate once inside ComputePriority but also twice
outside the function.  I'm not going to change this because I have more patches
in flight which touch this method.  But I'll keep it in mind next time I touch
this code to make it nicer.

>> ParticularProcessPriorityManager::SetPriorityNow(ProcessPriority aPriority)
>
>Can you assert that mContentParent is non-null here?

I don't like asserting that a variable is non-null right before we use it.

Also, if mContentParent was null, I think we'd crash on the value beforehand,
in ComputePriority.

>>   if (aPriority == PROCESS_PRIORITY_UNKNOWN) {
>>     MOZ_ASSERT(false);
>>     return;
>>   }
>
>If this isn't possible then just assert != PROCESS_PRIORITY_UNKNOWN, no runtime check.

It's not an invariant of this module; anyone can call ProcessPriorityManager::SetPriority.

Thanks for the review!  Getting ready to land this, but there are still pieces
that need to fall into place.

Comment 44

[Justin Lebar (not reading bugmail)]

That b2g18 patch was so beautiful and green (not); here's an m-c try push.  https://tbpl.mozilla.org/?tree=Try&rev=409a34ceaaa4

Comment 45

[Justin Lebar (not reading bugmail)]

bjacob: The try run above looks mostly fine, except for m2 failures.  I /think/ the log is complaining only about the one m2 child process crash; I don't think there's a parent process crash.

https://tbpl.mozilla.org/php/getParsedLog.php?id=22216994&tree=Try

The assertion is this one https://github.com/mozilla/mozilla-central/blob/5d79dd5e769265a8d6b3bc394a42c3ae52643920/gfx/layers/client/CompositableClient.cpp#L112

It's happening in my test_WebGLContextLost test, which opens a WebGL context in a child process after force-enabling WebGL.

This assertion happens locally, so I'd assumed that was something wrong with my machine.  But it's happening remotely, too.

I don't know what this assertion means, so if you have any insight, that would be appreciated!

I need to get a full try run on this code before pushing it, so if we can figure this assertion out before that finishes, great.  Otherwise I think I'm going to disable this test and worry about it in a follow-up.

Comment 46

[Benoit Jacob [:bjacob] (mostly away)]

I may not be the best expert about this code; you want :nical or :nrc or Bas for questions about TextureClient in general and :mattwoodrow and :jgilbert for the hooking up of WebGL into this.

That said: to understand what's going on here, I'd need to know the value of |aTextureClientType| and if possible a full stack with argument values. I'd also like to know the value of |parentBackend|.

If I had to guess, I'd suppose that aTextureClientType==TEXTURE_SHMEM and parentBackend != LAYERS_OPENGL.

We really need to understand this, and then you really need to ask one of the specialists listed above.

Comment 47

[Justin Lebar (not reading bugmail)]

> Program received signal SIGSEGV, Segmentation fault.
>
> (gdb) p aTextureClientType
> $1 = mozilla::layers::TEXTURE_SHMEM
>
> (gdb) bt
> #0  mozilla::layers::CompositableClient::CreateTextureClient (this=0x249f440, 
>     aTextureClientType=mozilla::layers::TEXTURE_SHMEM) at ../../../src/gfx/layers/client/CompositableClient.cpp:112
> #1  0x00002b1d0bce176d in mozilla::layers::CanvasClient2D::Update (this=0x249f440, aSize=..., aLayer=0x2678500)
>     at ../../../src/gfx/layers/client/CanvasClient.cpp:55
> #2  0x00002b1d0bc90751 in mozilla::layers::BasicShadowableCanvasLayer::Paint (this=0x2678500, aContext=0x24f2eb0, 
>     aMaskLayer=0x0) at ../../../src/gfx/layers/basic/BasicCanvasLayer.cpp:310
> #3  0x00002b1d0bc908e7 in non-virtual thunk to mozilla::layers::BasicShadowableCanvasLayer::Paint(gfxContext*, mozilla::layers::Layer*) (this=0x2678710, aContext=0x24f2eb0, aMaskLayer=0x0) at ../../../src/gfx/layers/basic/BasicCanvasLayer.cpp:316
> #4  0x00002b1d0bc8a3c8 in mozilla::layers::BasicLayerManager::PaintSelfOrChildren (this=0x2342140, aPaintContext=..., 
>     aGroupTarget=0x24f2eb0) at ../../../src/gfx/layers/basic/BasicLayerManager.cpp:832
> #5  0x00002b1d0bc8980e in mozilla::layers::BasicLayerManager::PaintLayer (this=0x2342140, aTarget=0x24f2eb0, 
>     aLayer=0x2678500, 
>     aCallback=0x2b1d09918180 <mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*)>, aCallbackData=0x7ffffe7a9f40, aReadback=0x7ffffe7a8e20)
>     at ../../../src/gfx/layers/basic/BasicLayerManager.cpp:976
> #6  0x00002b1d0bc8a4b4 in mozilla::layers::BasicLayerManager::PaintSelfOrChildren (this=0x2342140, aPaintContext=..., 
>     aGroupTarget=0x24f2eb0) at ../../../src/gfx/layers/basic/BasicLayerManager.cpp:844
> #7  0x00002b1d0bc8980e in mozilla::layers::BasicLayerManager::PaintLayer (this=0x2342140, aTarget=0x24f2eb0, 
>     aLayer=0x208c460, 
>     aCallback=0x2b1d09918180 <mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*)>, aCallbackData=0x7ffffe7a9f40, aReadback=0x0)
>     at ../../../src/gfx/layers/basic/BasicLayerManager.cpp:976
> #8  0x00002b1d0bc88700 in mozilla::layers::BasicLayerManager::EndTransactionInternal (this=0x2342140, 
>     aCallback=0x2b1d09918180 <mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*)>, aCallbackData=0x7ffffe7a9f40, 
>     aFlags=mozilla::layers::LayerManager::END_NO_COMPOSITE) at ../../../src/gfx/layers/basic/BasicLayerManager.cpp:590
> #9  0x00002b1d0bc87fca in mozilla::layers::BasicLayerManager::EndTransaction (this=0x2342140, 
>     aCallback=0x2b1d09918180 <mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*)>, aCallbackData=0x7ffffe7a9f40, 
>     aFlags=mozilla::layers::LayerManager::END_NO_COMPOSITE) at ../../../src/gfx/layers/basic/BasicLayerManager.cpp:509
> #10 0x00002b1d0bc8c05a in mozilla::layers::BasicShadowLayerManager::EndTransaction (this=0x2342140, 
>     aCallback=0x2b1d09918180 <mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*)>, aCallbackData=0x7ffffe7a9f40, 
>     aFlags=mozilla::layers::LayerManager::END_NO_COMPOSITE) at ../../../src/gfx/layers/basic/BasicLayerManager.cpp:1198
> #11 0x00002b1d0998f63b in nsDisplayList::PaintForFrame (this=0x7ffffe7a9f18, aBuilder=0x7ffffe7a9f40, aCtx=0x0, 
>     aForFrame=0x20d2e50, aFlags=13) at ../../../src/layout/base/nsDisplayList.cpp:1196
> #12 0x00002b1d0998eb2f in nsDisplayList::PaintRoot (this=0x7ffffe7a9f18, aBuilder=0x7ffffe7a9f40, aCtx=0x0, aFlags=13)
>     at ../../../src/layout/base/nsDisplayList.cpp:1058
> #13 0x00002b1d099db6ed in nsLayoutUtils::PaintFrame (aRenderingContext=0x0, aFrame=0x20d2e50, aDirtyRegion=..., aBackstop=0, 
>     aFlags=772) at ../../../src/layout/base/nsLayoutUtils.cpp:2092
> #14 0x00002b1d09a13b1f in PresShell::Paint (this=0x217f0d0, aViewToPaint=0x24e2760, aDirtyRegion=..., aFlags=1)
>     at ../../../src/layout/base/nsPresShell.cpp:5588
> #15 0x00002b1d0a4ff51e in nsViewManager::ProcessPendingUpdatesForView (this=0x220f8f0, aView=0x24e2760, 
>     aFlushDirtyRegion=true) at ../../../src/view/src/nsViewManager.cpp:395
> #16 0x00002b1d0a500404 in nsViewManager::ProcessPendingUpdates (this=0x220f8f0)
>     at ../../../src/view/src/nsViewManager.cpp:1116
> #17 0x00002b1d09a2df62 in nsRefreshDriver::Tick (this=0x23714f0, aNowEpoch=1366906161111260, aNowTime=...)
>     at ../../../src/layout/base/nsRefreshDriver.cpp:962
> #18 0x00002b1d09a30f3c in mozilla::RefreshDriverTimer::TickDriver (driver=0x23714f0, jsnow=1366906161111260, now=...)
>     at ../../../src/layout/base/nsRefreshDriver.cpp:164
> #19 0x00002b1d09a30e04 in mozilla::RefreshDriverTimer::Tick (this=0x23eab50)
>     at ../../../src/layout/base/nsRefreshDriver.cpp:156
> #20 0x00002b1d09a30cc1 in mozilla::RefreshDriverTimer::TimerTick (aTimer=0x23ece60, aClosure=0x23eab50)
>     at ../../../src/layout/base/nsRefreshDriver.cpp:181
> #21 0x00002b1d0bb690c5 in nsTimerImpl::Fire (this=0x23ece60) at ../../../src/xpcom/threads/nsTimerImpl.cpp:547
> #22 0x00002b1d0bb69571 in nsTimerEvent::Run (this=0x2b1d48000d90) at ../../../src/xpcom/threads/nsTimerImpl.cpp:634
> #23 0x00002b1d0bb5f5b6 in nsThread::ProcessNextEvent (this=0x1def240, mayWait=true, result=0x7ffffe7ab248)
>     at ../../../src/xpcom/threads/nsThread.cpp:627
> #24 0x00002b1d0bb96c2a in NS_InvokeByIndex (that=0x1def240, methodIndex=8, paramCount=2, params=0x7ffffe7ab230)
>     at ../../../../../../../src/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:162
> #25 0x00002b1d0acecb74 in CallMethodHelper::Invoke (this=0x7ffffe7ab1f0)
>     at ../../../../src/js/xpconnect/src/XPCWrappedNative.cpp:2945
> #26 0x00002b1d0ace907c in CallMethodHelper::Call (this=0x7ffffe7ab1f0)
>     at ../../../../src/js/xpconnect/src/XPCWrappedNative.cpp:2280
> #27 0x00002b1d0ace8e54 in XPCWrappedNative::CallMethod (ccx=..., mode=XPCWrappedNative::CALL_METHOD)
>     at ../../../../src/js/xpconnect/src/XPCWrappedNative.cpp:2246
> #28 0x00002b1d0acf8b36 in XPC_WN_CallMethod (cx=0x22f0100, argc=1, vp=0x2b1d284f72e0)
>     at ../../../../src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1483
> ---Type <return> to continue, or q <return> to quit---
> #29 0x00002b1d0cac276d in js::CallJSNative (cx=0x22f0100, 
>     native=0x2b1d0acf87a0 <XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)>, args=...)
>     at ../../../src/js/src/jscntxtinlines.h:337
> #30 0x00002b1d0cac2199 in js::InvokeKernel (cx=0x22f0100, args=..., construct=js::NO_CONSTRUCT)
>     at ../../../src/js/src/jsinterp.cpp:407
> #31 0x00002b1d0cabbc87 in js::Interpret (cx=0x22f0100, entryFrame=0x2b1d284f71a0, interpMode=js::JSINTERP_NORMAL, 
>     useNewType=false) at ../../../src/js/src/jsinterp.cpp:2396
> #32 0x00002b1d0cab4e67 in js::RunScript (cx=0x22f0100, fp=0x2b1d284f71a0) at ../../../src/js/src/jsinterp.cpp:364
> #33 0x00002b1d0cac226d in js::InvokeKernel (cx=0x22f0100, args=..., construct=js::NO_CONSTRUCT)
>     at ../../../src/js/src/jsinterp.cpp:421
> #34 0x00002b1d0c9ccfa3 in js::Invoke (cx=0x22f0100, args=..., construct=js::NO_CONSTRUCT)
>     at ../../../src/js/src/jsinterp.h:134
> #35 0x00002b1d0cac2c15 in js::Invoke (cx=0x22f0100, thisv=..., fval=..., argc=2, argv=0x2b1d284f7170, rval=0x2b1d284f7160)
>     at ../../../src/js/src/jsinterp.cpp:454
> #36 0x00002b1d0cb40ad5 in js::DirectProxyHandler::call (this=0x2b1d0ef1c408 <js::CrossCompartmentWrapper::singleton>, 
>     cx=0x22f0100, proxy=..., args=...) at ../../../src/js/src/jsproxy.cpp:481
> #37 0x00002b1d0cc2d38f in js::CrossCompartmentWrapper::call (this=0x2b1d0ef1c408 <js::CrossCompartmentWrapper::singleton>, 
>     cx=0x22f0100, wrapper=..., args=...) at ../../../src/js/src/jswrapper.cpp:453
> #38 0x00002b1d0cb4f24a in js::Proxy::call (cx=0x22f0100, proxy=..., args=...) at ../../../src/js/src/jsproxy.cpp:2613
> #39 0x00002b1d0cb51b43 in proxy_Call (cx=0x22f0100, argc=2, vp=0x2b1d284f7160) at ../../../src/js/src/jsproxy.cpp:3177
> #40 0x00002b1d0cac276d in js::CallJSNative (cx=0x22f0100, 
>     native=0x2b1d0cb51a50 <proxy_Call(JSContext*, unsigned int, JS::Value*)>, args=...)
>     at ../../../src/js/src/jscntxtinlines.h:337
> #41 0x00002b1d0cac20ce in js::InvokeKernel (cx=0x22f0100, args=..., construct=js::NO_CONSTRUCT)
>     at ../../../src/js/src/jsinterp.cpp:400
> #42 0x00002b1d0cabbc87 in js::Interpret (cx=0x22f0100, entryFrame=0x2b1d284f70f0, interpMode=js::JSINTERP_NORMAL, 
>     useNewType=false) at ../../../src/js/src/jsinterp.cpp:2396
> #43 0x00002b1d0cab4e67 in js::RunScript (cx=0x22f0100, fp=0x2b1d284f70f0) at ../../../src/js/src/jsinterp.cpp:364
> #44 0x00002b1d0cac226d in js::InvokeKernel (cx=0x22f0100, args=..., construct=js::NO_CONSTRUCT)
>     at ../../../src/js/src/jsinterp.cpp:421
> #45 0x00002b1d0c9ccfa3 in js::Invoke (cx=0x22f0100, args=..., construct=js::NO_CONSTRUCT)
>     at ../../../src/js/src/jsinterp.h:134
> #46 0x00002b1d0cac2c15 in js::Invoke (cx=0x22f0100, thisv=..., fval=..., argc=2, argv=0x7ffffe7b1390, rval=0x7ffffe7b1030)
>     at ../../../src/js/src/jsinterp.cpp:454
> #47 0x00002b1d0c9a388f in JS_CallFunctionValue (cx=0x22f0100, objArg=0x2b1d34351b00, fval=..., argc=2, argv=0x7ffffe7b1390, 
>     rval=0x7ffffe7b1030) at ../../../src/js/src/jsapi.cpp:5891
> #48 0x00002b1d0acdc18a in nsXPCWrappedJSClass::CallMethod (this=0x217e710, wrapper=0x25f9690, methodIndex=3, 
>     info_=0x1ea72f8, nativeParams=0x7ffffe7b1730) at ../../../../src/js/xpconnect/src/XPCWrappedJSClass.cpp:1436
> #49 0x00002b1d0accdf14 in nsXPCWrappedJS::CallMethod (this=0x25f9690, methodIndex=3, info=0x1ea72f8, params=0x7ffffe7b1730)
>     at ../../../../src/js/xpconnect/src/XPCWrappedJS.cpp:578
> #50 0x00002b1d0bb980db in PrepareAndDispatch (self=0x25f9720, methodIndex=3, args=0x7ffffe7b1880, gpregs=0x7ffffe7b1800, 
>     fpregs=0x7ffffe7b1830) at ../../../../../../../src/xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:122
> #51 0x00002b1d0bb97113 in SharedStub () from /home/jlebar/code/moz/ff-git/debug/dist/bin/libxul.so
> #52 0x00002b1d0a55d0f4 in nsGlobalWindow::Alert (this=0x2322eb0, aString=...)
>     at ../../../src/dom/base/nsGlobalWindow.cpp:5282
> #53 0x00002b1d0a55cad9 in nsGlobalWindow::Alert (this=0x24d7f00, aString=...)
>     at ../../../src/dom/base/nsGlobalWindow.cpp:5219
> #54 0x00002b1d0a55d1af in non-virtual thunk to nsGlobalWindow::Alert(nsAString_internal const&) (this=0x24d7f18, aString=...)
>     at ../../../src/dom/base/nsGlobalWindow.cpp:5286
> #55 0x00002b1d0bb96c2a in NS_InvokeByIndex (that=0x24d7f18, methodIndex=33, paramCount=1, params=0x7ffffe7b20b0)
>     at ../../../../../../../src/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:162
> #56 0x00002b1d0acecb74 in CallMethodHelper::Invoke (this=0x7ffffe7b2070)
>     at ../../../../src/js/xpconnect/src/XPCWrappedNative.cpp:2945
> #57 0x00002b1d0ace907c in CallMethodHelper::Call (this=0x7ffffe7b2070)
>     at ../../../../src/js/xpconnect/src/XPCWrappedNative.cpp:2280
> #58 0x00002b1d0ace8e54 in XPCWrappedNative::CallMethod (ccx=..., mode=XPCWrappedNative::CALL_METHOD)
>     at ../../../../src/js/xpconnect/src/XPCWrappedNative.cpp:2246
> #59 0x00002b1d0acf8b36 in XPC_WN_CallMethod (cx=0x22f0100, argc=1, vp=0x2b1d284f70b8)
>     at ../../../../src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1483
> #60 0x00002b1d0cac276d in js::CallJSNative (cx=0x22f0100, 
>     native=0x2b1d0acf87a0 <XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)>, args=...)
>     at ../../../src/js/src/jscntxtinlines.h:337
> #61 0x00002b1d0cac2199 in js::InvokeKernel (cx=0x22f0100, args=..., construct=js::NO_CONSTRUCT)
>     at ../../../src/js/src/jsinterp.cpp:407
> #62 0x00002b1d0cabbc87 in js::Interpret (cx=0x22f0100, entryFrame=0x2b1d284f7038, interpMode=js::JSINTERP_NORMAL, 
>     useNewType=false) at ../../../src/js/src/jsinterp.cpp:2396
> #63 0x00002b1d0cab4e67 in js::RunScript (cx=0x22f0100, fp=0x2b1d284f7038) at ../../../src/js/src/jsinterp.cpp:364
> #64 0x00002b1d0cac226d in js::InvokeKernel (cx=0x22f0100, args=..., construct=js::NO_CONSTRUCT)
> ---Type <return> to continue, or q <return> to quit---
>     at ../../../src/js/src/jsinterp.cpp:421
> #65 0x00002b1d0c9ccfa3 in js::Invoke (cx=0x22f0100, args=..., construct=js::NO_CONSTRUCT)
>     at ../../../src/js/src/jsinterp.h:134
> #66 0x00002b1d0cac2c15 in js::Invoke (cx=0x22f0100, thisv=..., fval=..., argc=0, argv=0x7ffffe7b4e18, rval=0x7ffffe7b4e68)
>     at ../../../src/js/src/jsinterp.cpp:454
> #67 0x00002b1d0c9a388f in JS_CallFunctionValue (cx=0x22f0100, objArg=0x2b1d3453c080, fval=..., argc=0, argv=0x7ffffe7b4e18, 
>     rval=0x7ffffe7b4e68) at ../../../src/js/src/jsapi.cpp:5891
> #68 0x00002b1d0b7abb3e in mozilla::dom::Function::Call (this=0x2245730, cx=0x22f0100, aThisObj=0x2b1d3453c080, 
>     arguments=..., aRv=...) at FunctionBinding.cpp:49
> #69 0x00002b1d0a578191 in mozilla::dom::Function::Call<nsCOMPtr<nsISupports> > (this=0x2245730, thisObj=..., arguments=..., 
>     aRv=..., aExceptionHandling=mozilla::dom::CallbackObject::eReportExceptions)
>     at ../../dist/include/mozilla/dom/FunctionBinding.h:45
> #70 0x00002b1d0a577fb0 in nsGlobalWindow::RunTimeoutHandler (this=0x24d7f00, aTimeout=0x22404d0, aScx=0x1de4940)
>     at ../../../src/dom/base/nsGlobalWindow.cpp:10199
> #71 0x00002b1d0a5682fe in nsGlobalWindow::RunTimeout (this=0x24d7f00, aTimeout=0x22404d0)
>     at ../../../src/dom/base/nsGlobalWindow.cpp:10441
> #72 0x00002b1d0a577920 in nsGlobalWindow::TimerCallback (aTimer=0x2245900, aClosure=0x22404d0)
>     at ../../../src/dom/base/nsGlobalWindow.cpp:10710
> #73 0x00002b1d0bb690c5 in nsTimerImpl::Fire (this=0x2245900) at ../../../src/xpcom/threads/nsTimerImpl.cpp:547
> #74 0x00002b1d0bb69571 in nsTimerEvent::Run (this=0x2b1d48000dc0) at ../../../src/xpcom/threads/nsTimerImpl.cpp:634
> #75 0x00002b1d0bb5f5b6 in nsThread::ProcessNextEvent (this=0x1def240, mayWait=false, result=0x7ffffe7b55ce)
>     at ../../../src/xpcom/threads/nsThread.cpp:627
> #76 0x00002b1d0bacccd9 in NS_ProcessNextEvent (thread=0x1def240, mayWait=false)
>     at ../../../src/xpcom/glue/nsThreadUtils.cpp:238
> #77 0x00002b1d0b49498d in mozilla::ipc::MessagePump::Run (this=0x1da5810, aDelegate=0x7ffffe7b5970)
>     at ../../../src/ipc/glue/MessagePump.cpp:82
> #78 0x00002b1d0b495056 in mozilla::ipc::MessagePumpForChildProcess::Run (this=0x1da5810, aDelegate=0x7ffffe7b5970)
>     at ../../../src/ipc/glue/MessagePump.cpp:231
> #79 0x00002b1d0bbd3d46 in MessageLoop::RunInternal (this=0x7ffffe7b5970)
>     at ../../../src/ipc/chromium/src/base/message_loop.cc:219
> #80 0x00002b1d0bbd3cc5 in MessageLoop::RunHandler (this=0x7ffffe7b5970)
>     at ../../../src/ipc/chromium/src/base/message_loop.cc:212
> #81 0x00002b1d0bbd3c9d in MessageLoop::Run (this=0x7ffffe7b5970) at ../../../src/ipc/chromium/src/base/message_loop.cc:186
> #82 0x00002b1d0b28ea91 in nsBaseAppShell::Run (this=0x224d130) at ../../../src/widget/xpwidgets/nsBaseAppShell.cpp:163
> #83 0x00002b1d093ee3e1 in XRE_RunAppShell () at ../../../src/toolkit/xre/nsEmbedFunctions.cpp:659
> #84 0x00002b1d0b494ef8 in mozilla::ipc::MessagePumpForChildProcess::Run (this=0x1da5810, aDelegate=0x7ffffe7b5970)
>     at ../../../src/ipc/glue/MessagePump.cpp:198
> #85 0x00002b1d0bbd3d46 in MessageLoop::RunInternal (this=0x7ffffe7b5970)
>     at ../../../src/ipc/chromium/src/base/message_loop.cc:219
> #86 0x00002b1d0bbd3cc5 in MessageLoop::RunHandler (this=0x7ffffe7b5970)
>     at ../../../src/ipc/chromium/src/base/message_loop.cc:212
> #87 0x00002b1d0bbd3c9d in MessageLoop::Run (this=0x7ffffe7b5970) at ../../../src/ipc/chromium/src/base/message_loop.cc:186
> #88 0x00002b1d093eda7a in XRE_InitChildProcess (aArgc=3, aArgv=0x7ffffe7b6ef8, aProcess=GeckoProcessType_Content)
>     at ../../../src/toolkit/xre/nsEmbedFunctions.cpp:496
> #89 0x000000000040132c in main (argc=5, argv=0x7ffffe7b6ef8) at ../../../src/ipc/app/MozillaRuntimeMain.cpp:60

I tried to dump a JS stack and it didn't give me anything.

Comment 48

[Justin Lebar (not reading bugmail)]

First writer wins, I suppose.  Please see comment 45.

Comment 49

[Benoit Jacob [:bjacob] (mostly away)]

Can you also print |parentBackend| ?

Comment 50

[Justin Lebar (not reading bugmail)]

In the meantime, here's another try push, which I expect to have this particular m2 failure.

https://tbpl.mozilla.org/?tree=Try&rev=13f4b49bce97

Comment 51

[Justin Lebar (not reading bugmail)]

(In reply to Benoit Jacob [:bjacob] from comment #49)
> Can you also print |parentBackend| ?

(gdb) p parentBackend
$2 = mozilla::layers::LAYERS_NONE

Comment 52

[Alex Keybl [:akeybl]]

We'd like to target this bug for 5/10 given the platform risk here. Please let us know if that's an unreasonable target.

Comment 53

[Nicolas Silva [:nical]]

It's bad that the layers backend is LAYERS_NONE, we should try to catch it earlier in TabChild::InitRenderingState and nsBaseWidget::CreateCompositor.

I need to look at it some more.

Comment 54

[Nicolas Silva [:nical]]

Attachment: (optional) Assert early that we have a valid layers backend

Just putting that there, depending on which assertion blows first, it can give valuable info about in which side the problem is and/or if things are initialized in the wrong order.

Not sure it should be landed as a fatal assertion though.

Comment 55

[Justin Lebar (not reading bugmail)]

> Since this is only supposed to be called by PreallocatedProcessManager would it make sense to make 
> PreallocatedProcessManager a friend and hide this in a private section?

Except it's called by PreallocatedProcessManagerImpl, which is now in an anonymous namespace.  :)  So that doesn't work.

Comment 56

[Justin Lebar (not reading bugmail)]

> Just putting that there, depending on which assertion blows first, it can give valuable info about 
> in which side the problem is and/or if things are initialized in the wrong order.

With this patch we fail my first OOP test (no webgl involved) with the assertion in TabChild.

Comment 57

[Nick Cameron [:nrc]]

(In reply to Justin Lebar [:jlebar] from comment #56)
> > Just putting that there, depending on which assertion blows first, it can give valuable info about 
> > in which side the problem is and/or if things are initialized in the wrong order.
> 
> With this patch we fail my first OOP test (no webgl involved) with the
> assertion in TabChild.

So the thing to do will be to trace through that |SendPRenderFrameConstructor| call and find out why we are not getting a valid compositor backend, which will explain why we can't create texture clients later on.

Comment 58

[Justin Lebar (not reading bugmail)]

Do you think we have to block this bug on investigating this layers issue?  If so, we need to resolve the issue right away, because this bug is of the highest priority class we have in B2G.

Either way, I'm under the gun to get a lot of stuff done very quickly right now, so I'd appreciate if someone who actually knows what's going on here could investigate this.

Comment 59

[Nicolas Silva [:nical]]

(In reply to Justin Lebar [:jlebar] from comment #58)
> Do you think we have to block this bug on investigating this layers issue? 
> If so, we need to resolve the issue right away, because this bug is of the
> highest priority class we have in B2G.
> 
> Either way, I'm under the gun to get a lot of stuff done very quickly right
> now, so I'd appreciate if someone who actually knows what's going on here
> could investigate this.

I understand, I'm setting things up to reproduce on my machine, and help you with this.

Comment 60

[Justin Lebar (not reading bugmail)]

> Do you think we have to block this bug on investigating this layers issue? 

I don't understand the criticality of this layers issue; should it block us from landing these patches?  I'm essentially ready to push now...

Comment 61

[Justin Lebar (not reading bugmail)]

I split the layers issue into bug 865844.  Still waiting to hear whether I can land this safely without a fix there.

Comment 62

[Benoit Jacob [:bjacob] (mostly away)]

I am less authoritative than :nrc or :nical here, but I think that having filed this follow-up is all you needed to do to be allowed to proceed with landing this without a fix.

Comment 63

[Justin Lebar (not reading bugmail)]

I've disabled the tests on everything except Linux, and I've also disabled the webgl test (bug 865844).  I think I fixed the xpcshell crash.  So this is a linux-only try push, and if it's green, I'll push to inbound.

Comment 64

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/projects/birch/rev/77960f39133f
https://hg.mozilla.org/projects/birch/rev/38fee265dbe5
https://hg.mozilla.org/projects/birch/rev/a5628782c872
https://hg.mozilla.org/projects/birch/rev/e8e47474ef41
https://hg.mozilla.org/projects/birch/rev/911d21581db4
https://hg.mozilla.org/projects/birch/rev/1f675d6e1783
https://hg.mozilla.org/projects/birch/rev/51bf50daff2f

Comment 65

[Justin Lebar (not reading bugmail)]

This needs some non-trivial love to backport; don't worry about trying, sheriffs.

Comment 66

[Nicolas Silva [:nical]]

(In reply to Justin Lebar [:jlebar] from comment #65)
> This needs some non-trivial love to backport; don't worry about trying,
> sheriffs.

Indeed. I started rebasing this patch queue on top of current m-c yesterday but it's not trivial so I stopped half-way through. I'll wait for you to do the rebase if it's not a problem.

Comment 67

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/84787582d353
https://hg.mozilla.org/mozilla-central/rev/e2ab897e0241
https://hg.mozilla.org/mozilla-central/rev/7b93ed92f8fe
https://hg.mozilla.org/mozilla-central/rev/77960f39133f
https://hg.mozilla.org/mozilla-central/rev/38fee265dbe5
https://hg.mozilla.org/mozilla-central/rev/a5628782c872
https://hg.mozilla.org/mozilla-central/rev/e8e47474ef41

Comment 68

[Justin Lebar (not reading bugmail)]

Backporting to b2g18 is blocked on the backport of bug 852847.

Comment 69

[Justin Lebar (not reading bugmail)]

I think there's a bug here which is causing us to send child processes into the background when we shouldn't.  I need to figure out why this isn't being caught by my tests...

Comment 70

[Justin Lebar (not reading bugmail)]

Attachment: Follow-up: Fix typo in BrowserElementChildPreload that was breaking bg --> fg transitions in childVisibilityChanged.

Also modify the test so it catches this mistake.

Gregor, I think this may fix the spurrious renice'ing that you were seeing.  At the very least, you shouldn't see that "setting visibility to 'undefined'" anymore.  :)

Comment 71

[Daniel Coloma:dcoloma]

Justin, should I mark status for v1.0.1 and v1-train as affected or do you prefer to wait until the follow-up is landed?

Comment 72

[Justin Lebar (not reading bugmail)]

(In reply to Daniel Coloma:dcoloma from comment #71)
> Justin, should I mark status for v1.0.1 and v1-train as affected or do you
> prefer to wait until the follow-up is landed?

Go for it; we're waiting on bug 852847 either way.

Comment 73

[Justin Lebar (not reading bugmail)]

Pushed the follow-up to birch.  Re-opening so we can close this when the follow-up makes it to m-c.

https://hg.mozilla.org/projects/birch/rev/a2e4282b32c0

Comment 74

[Justin Lebar (not reading bugmail)]

b2g18 try: https://tbpl.mozilla.org/?tree=Try&rev=fe21d7cfe568

Comment 75

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a2e4282b32c0

Comment 76

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/f9be10ef7432
https://hg.mozilla.org/releases/mozilla-b2g18/rev/1ae619a566f0
https://hg.mozilla.org/releases/mozilla-b2g18/rev/b26d56732a52
https://hg.mozilla.org/releases/mozilla-b2g18/rev/b28b4942d616
https://hg.mozilla.org/releases/mozilla-b2g18/rev/96f36baa9e2d
https://hg.mozilla.org/releases/mozilla-b2g18/rev/45491a6aad33
https://hg.mozilla.org/releases/mozilla-b2g18/rev/fa37c1944663
https://hg.mozilla.org/releases/mozilla-b2g18/rev/5f5106142c17

Comment 77

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/106ecc4b3f85
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/eba35d5d6e30
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/5b5c8c46e5bf
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/21b21a1c23be
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/8cd76b3544c7
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/2dc2707444c5
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/aa607e3f4c4a
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/f9d1e1c946df