1529735 - How should sCCRunner and sICCRunner interact?

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Boris Zbarsky [:bzbarsky]]

Before the patch for bug 1377131 the code in nsJSContext::RunNextCollectorTimer looked like this:

  if (sCCRunner) {
    if (ReadyToTriggerExpensiveCollectorTimer()) {
      CCRunnerFired(TimeStamp());
    }
    return;
  }
 
  if (sICCRunner) {
    ICCRunnerFired(TimeStamp());
    return;
  }

but after that bug (and on tip) it looks like this:

  if (sCCRunner) {
    sCCRunner->SetDeadline(aDeadline);
    runnable = sCCRunner;
  }

  if (sICCRunner) {
    sICCRunner->SetDeadline(aDeadline);
    runnable = sICCRunner;
  }

  if (runnable) {
    runnable->Run();
  }

In particular, of both sCCRunner and sICCRunner are non-null, we used to do CCRunnerFired() and return. But now we'll run sICCRunner. Is that an intended change in behavior?

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

sICCRunner should be null when sCCRunner is non-null.
sCCRunner does all the forget skippable handling, and eventually may trigger iCC, which then
sets sCCRunner to null and set sICCRunner to non-null
https://searchfox.org/mozilla-central/rev/dc0adc07db3df9431a0876156f50c65d580010cb/dom/base/nsJSEnvironment.cpp#1534-1548.

Comment 2

[Boris Zbarsky [:bzbarsky]]

sICCRunner should be null when sCCRunner is non-null.

Is that worth asserting?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1529735, assert that only either sCCRunner or sICCRunner is active, not both, r=mccr8!

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Hopefully the assertion doesn't fire
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9d473b320bd0b18db3fd9e5b8bcdf28f9196fed

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3dc67a87eb4
assert that only either sCCRunner or sICCRunner is active, not both, r=mccr8

Comment 6

[Cosmin Sabou [:CosminS]]

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