811459 - nsIdleService continuously reports both "active" and "idle" 5 seconds after going idle

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Michael Kraft [:morac]]

Starting in the 2012-11-03 nightly (and Aurora) load, the nsIdleService has not been working correctly.  The 2012-11-02 load works correctly.

What happens is that after the specified number of seconds (x) an "idle" notification is correctly sent, but after MIN_IDLE_POLL_INTERVAL_MSEC (i.e. 5) additional seconds the system starts sending both "active" and "idle" notifications dozens of times a second until the system is no longer idle.

This is almost certainly caused by bug 803039 since that was pushed out on 2012-11-02.


Steps to reproduce simply copy and paste the following into the error console:

var idleService = Components.classes["@mozilla.org/widget/idleservice;1"].getService(Components.interfaces.nsIIdleService); var idleObserver = {   observe: function(subject, topic, data) { Components.utils.reportError("topic: " + topic + "\ndata: " + data); } }; idleService.addIdleObserver(idleObserver, 2);


Expected result:

The error console should display the following on idle:
topic: idle
data: 2

and the following when active:
topic: active
data: 0

Actual results:

topic: idle
data: 2
topic: active
data: 7
...
topic: idle
data: 7
topic: active
data: 8
...
topic: idle
data: 8
topic: active
data: 9
...
topic: idle
data: 9
topic: active
data: 10
topic: idle
...
data: 10
...
topic: active
data: 0

Comment 1

[Justin Lebar (not reading bugmail)]

Shelly, this may be a regression which affects desktop builds.  Can you please treat this as a priority and look at it soon?

Comment 2

[Michael Kraft [:morac]]

I tried to reproduce this tonight on my home PC with the 2012-11-14 nightly and could not.  I can still reproduce it in the Aurora 2012-11-14 load though.  

I had reproduced it in the 2012-11-13 nightly load on my work PC.  I'm not sure what changed between yesterday and today, but I didn't see any changes in the nsIdleService function.   I'm puzzled by this since I'm almost positive bug  	803039 was the cause simply based on the timing of when the problem started.

Comment 3

[Michael Kraft [:morac]]

Scratch my last comment.  I couldn't reproduce it immediately after updating to the 2012-11-14 nightly.  After I ran Aurora and switched back to the nightly, I could reproduce it again.

To make doubly sure, I wiped my profile and created a new one and I could still reproduce it in the 2012-11-14 nightly.  I have no idea why I couldn't reproduce it originally.

Comment 4

[Scoobidiver (away)]

Fixed by the backout of bug 803039:
https://hg.mozilla.org/releases/mozilla-aurora/rev/5516a682b842
http://hg.mozilla.org/mozilla-central/rev/5b72b916c65d

Comment 5

[Shelly Lin [:shelly]]

Attachment: v1: IdleService fix

After staring at the log for hours, I finally addressed the issue. It is cause by a type conversion rounding problem, where before:

At the line in function nsIdleService::IdleTimerCallback(void)

> if (aIdleTime.ToMilliseconds() > currentIdleTimeInMS)

According to the comment, this logic is a safety check if we have some user interaction we didn't handle previously, in most normal case, it shouldn't fall through because aIdleTime.ToMilliseconds() should be the same as currentIdleTimeInMS. However, a type conversion of TimeStamp.ToMilliseconds() makes aIdleTime.ToMilliseconds() larger than currentIdleTimeInMS by 1 millisecond, cause system to reset the timeout, and thus make all listeners non-idle. I've verified this also fixed bug 812378.

Hi Justin, if you think the fix is okay, I'll put some comments in the code to explain all above, thanks :)

Comment 6

[Justin Lebar (not reading bugmail)]

sigh.  We really need to rewrite this code.  It is a mess.

Thanks for figuring this out, Shelly.

I'm trying to figure out why it's safe to have 10ms in your patch.  Would the idle service operate correctly if you had 1,000ms there, or if you had infinity there?  Unless 1,000ms and infinity would be correct, I don't see why 10ms is correct.  But I admit to being pretty confused here...

Comment 7

[Shelly Lin [:shelly]]

The number 10 is just a magic number, it is a terrible check of "Only true if we think A is "really" larger than B" (as for the statement "A - B > 10"), it could be some other number. I really don't like the fix either, but in the mean time, I can't think of a way to work around this problem, any better solution?

Comment 8

[Justin Lebar (not reading bugmail)]

> The number 10 is just a magic number, it is a terrible check of "Only true if we think A is "really" 
> larger than B" (as for the statement "A - B > 10"), it could be some other number.

I'm trying to understand whether this introduces another bug.  What happens if A > B but A - B < 10?  Can you show that the code behaves correctly in that case?

Comment 9

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

We could avoid the rounding error if we use TimeStamp everywhere internally. Or truncate the fractional part unconditionally, or set the number to the limit of the rounding error (I think it is one)

Comment 10

[Shelly Lin [:shelly]]

I got the latest code base(from yesterday, and it's already backout to the previous one, which used PRTime) and apply the patch from bug 803039, and now I couldn't reproduce the issue. 

I'm building a firefox browser and will test this issue asap.

Comment 11

[Shelly Lin [:shelly]]

Okay, I couldn't repro because my dialer was broken. 

Thanks Kan-Ru, so we can either do:
> if (aIdleTime.ToSeconds() > currentIdleTimeInS)

or

> if (aIdleTime.ToMilliseconds() - currentIdleTimeInMS > 10)

To avoid the rounding error.

If you're interested, here is a detail of how we get into an infinite loop because of the rounding error.

(1) If we have an existed idle observer(mAnyObserverIdle is true), calling ResetIdleTimeOut fires a callback immediately.

> NS_IMETHODIMP
> nsIdleService::ResetIdleTimeOut(uint32_t idleDeltaInMS)
> {
>   // Store the time
>   mLastUserInteraction = TimeStamp::Now() -
>                          TimeDuration::FromMilliseconds(idleDeltaInMS);
> 
>   // If no one is idle, then we are done, any existing timers can keep running.
>   if (!mAnyObserverIdle) {
>     return NS_OK;
>   }

(2) Adding an idle observer resets the mDeltaToNextIdleSwitchInS to 1 sec on B2G, 2 secs in the above case for browser.

(3) So, adding an idle observer LATER than the idle time, tells the system we have someone idle. (Make mAnyObserverIdle = true)

And because of the rounding error, it resets the timeout and fires a call back immediately, this is how we get into the infinite loop.

Callback.1 (mAnyObserverIdle = true)
  ->ResetTimeoue (Due to rounding error, set mAnyObserverIdle = false, and fire another Callback.2)
Callback.1 (set mAnyObserverIdle = true, due to (3))

Callback.2 (mAnyObserverIdle = true)
 ->ResetTimeoue (Due to rounding error, set mAnyObserverIdle = false, and fire another Callback.3)
Callback.2 (set mAnyObserverIdle = true, due to (3))

Callback.3 (mAnyObserverIdle = true)
 ->ResetTimeoue (Due to rounding error, set mAnyObserverIdle = false, and fire another Callback.4)
Callback.3 (set mAnyObserverIdle = true, due to (3))
...

Comment 12

[Justin Lebar (not reading bugmail)]

> if (aIdleTime.ToSeconds() > currentIdleTimeInS)

ToSeconds() returns a double, so you'd have to round this down to the nearest second or something, right?  Otherwise I don't see how this fixes the problem.

> if (aIdleTime.ToMilliseconds() - currentIdleTimeInMS > 10)

I think Kan-Ru was suggesting 1 instead of 10 here.  Or again, rounding ToMilliseconds down.

Comment 13

[Shelly Lin [:shelly]]

Hi Justin, Kan-Ru and I found out that according to the previous revision of IdleService.cpp, we should calculate the last detected idle time prior than calling GetIdleTime(), which make more sense of detecting whether we have a user activity by |hasUserActivity = lastIdleTime > currentGetIdleTime|.

Please see the following for more detail:
http://hg.mozilla.org/mozilla-central/file/87956/widget/xpwidgets/nsIdleService.cpp#l302

Thus, we should be really using:

if (aIdleTime.ToMilliseconds() <= currentIdleTimeInMS)

Thanks.

Comment 14

[Shelly Lin [:shelly]]

Or move up the logic of calculating aIdleTime before GetIdleTime(), and do 
if (aIdleTime.ToMilliseconds() > currentIdleTimeInMS).

Comment 15

[Shelly Lin [:shelly]]

Attachment: v2: IdleService fix

And rounding the ToMilliseconds() down first.

Comment 16

[Justin Lebar (not reading bugmail)]

Comment on attachment 684273 [details] [diff] [review]
v2: IdleService fix

Okay, I think this has a good chance of working properly.

But we need to restructure the patch a bit.  In particular:

* Declare variables closer to where they're going to be used.
* Don't declare unnecessary variables (we don't need lastIdleTime, as far as I can tell).

Comment 17

[Shelly Lin [:shelly]]

Attachment: v3: IdleService fix and clean up

Thanks Justin, this is a clean up version of the previous one.

Comment 18

[Justin Lebar (not reading bugmail)]

Did you mean to invert the inequality here?  Comparing v2 to v3, that doesn't look quite right.

Comment 19

[Shelly Lin [:shelly]]

(In reply to Justin Lebar [:jlebar] from comment #16)
> Comment on attachment 684273 [details] [diff] [review]
Hi Justin,
> v2: IdleService fix
> 
> Okay, I think this has a good chance of working properly.
> 
> But we need to restructure the patch a bit.  In particular:
> 
> * Declare variables closer to where they're going to be used.
So I pull the line of:
TimeDuration lastIdleTime = TimeStamp::Now() - mLastUserInteraction;
above the if statement, as a result, it is computed after GetIdleTime and it is not the last idle time anymore, this is why I invert the inequality.
> * Don't declare unnecessary variables (we don't need lastIdleTime, as far as
> I can tell).
Just making sure, did you mean lastIdleTimeInMS?

Thanks.

Comment 20

[Justin Lebar (not reading bugmail)]

>> we don't need lastIdleTime, as far as I can tell
> Just making sure, did you mean lastIdleTimeInMS?

I meant lastIdleTime -- I was thinking that instead of

> TimeDuration lastIdleTime = TimeStamp::Now() - mLastUserInteraction;
> uint32_t lastIdleTimeInMS = lastIdleTime.ToMilliseconds();

you could do

> uint32_t lastIdleTimeInMS = (TimeStamp::Now - mLastUserInteraction).ToMilliseconds();

But you could also get rid of lastIdleTimeInMS instead; I guess I should have
said that you don't need both.


I'm still really confused about this inequality.  Can we back up and look at
the original code and comapre that to this patch?  I'm going to try to be
precise here so there's no confusion, because I'm having a very hard time
keeping this straight.

The current code in nsIdleService.cpp does

> if (((PR_Now() - mLastUserInteractionInPR) / PR_USEC_PER_MSEC) >
>     currentIdleTimeInMS)

We could rewrite this using TimeStamps as

> if (static_cast<uint32_t>((TimeStamp::Now() - mLastUserInteraction).ToMilliseconds()) >
>     currentIdleTimeInMS)

The code in patch v3 effectively does

> if (currentIdleTimeInMS >
>     (uint32_t)(TimeStamp::Now() - mLastUserInteraction).ToMilliseconds())

Which is the opposite of the two bits of code above.

Are you saying that the current code (first chunk of code here) is wrong?  Otherwise I don't understand how patch v3 is correct.


Now, if I look at patch v2, it's a different story, because you're calculating lastIdleTime before you call GetIdleTime().  Let's figure this one out.

In patch v2, we do the following pseudocode:

> lastIdleTimeMS = (uint32_t) Now() - mLastUserInteraction
> currentIdleTimeMS = GetIdleTime()
>                   = Min((uint32_t)(Now() - mLastUserInteraction),
>       	          polledIdleTime)
>
> if (lastIdleTimeMS > currentIdleTimeMS)

Again, we compare this to the original code, which does

> if (((PR_Now() - mLastUserInteractionInPR) / PR_USEC_PER_MSEC) >
>     currentIdleTimeInMS)

which is equivalent to

> currentIdleTimeMS = GetIdleTime()
> lastIdleTime = (uint32_t) Now() - mLastUserInteraction
> if (lastIdleTime > currentIdleTime)

So it seems to me that the original code is not equivalent to patch v2.  It's
almost equivalent, except for the polledIdleTime case in the Min statement in
GetIdleTime().  But I think the polledIdleTime case is the whole point of this
if statement; otherwise, it's always comparing

  Now() - mLastUserInteraction (taken at time A)

to

  Now() - mLastUserInteraction (taken at time B, for the same mLastUserInteraction value as at time A).


It seems to me that the way to retain the code's original behavior is to do

> if (static_cast<uint32_t>((TimeStamp::Now() - mLastUserInteraction).ToMilliseconds()) >
>     currentIdleTimeInMS)

Unless the original code is buggy?

Comment 21

[Justin Lebar (not reading bugmail)]

Gah, sorry, I made a mistake talking about patch v2.  Ignore this part:

> So it seems to me that the original code is not equivalent to patch v2.  It's
> almost equivalent, except for the polledIdleTime case in the Min statement in
> GetIdleTime().  But I think the polledIdleTime case is the whole point of this
> if statement; otherwise, it's always comparing
> 
>   Now() - mLastUserInteraction (taken at time A)
> 
> to
> 
>   Now() - mLastUserInteraction (taken at time B, for the same mLastUserInteraction value as at time A).

and replace it with:

v2 is in fact equivalent to the original code, if we ignore the ordering of the Now() calls.  And I think the rounding down to the nearest MS is intended to make order of the Now() calls irrelevant.

So v2 still looks right to me, although I'd probably prefer to go with the more obviously correct change of

> if (static_cast<uint32_t>((TimeStamp::Now() - mLastUserInteraction).ToMilliseconds()) >
>     currentIdleTimeInMS)

unless there's some reason not to do that.

Comment 22

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

The original code is buggy. The semantic of the inequality was inverted in the refactoring of r87957

r87956:
  lastTime = Now() - mLastHandledActivity
  idleTime = GetIdleTime()
  userActivity = lastTime > idleTime

r87957 and later:
  idleTime = GetIdleTime()
  lastTime = Now() - mLastHandledActivity
  userActivity = lastTime > idleTime

The inequality is unchanged but the timing we GetIdleTime() is different.

Comment 23

[Shelly Lin [:shelly]]

(In reply to Justin Lebar [:jlebar] from comment #20)
> >> we don't need lastIdleTime, as far as I can tell
> > Just making sure, did you mean lastIdleTimeInMS?
> 
> I meant lastIdleTime -- I was thinking that instead of
> 
> > TimeDuration lastIdleTime = TimeStamp::Now() - mLastUserInteraction;
> > uint32_t lastIdleTimeInMS = lastIdleTime.ToMilliseconds();
> 
> you could do
> 
> > uint32_t lastIdleTimeInMS = (TimeStamp::Now - mLastUserInteraction).ToMilliseconds();
> 
> But you could also get rid of lastIdleTimeInMS instead; I guess I should have
> said that you don't need both.
> 

Hi Justin, thanks for your clarification, it's crystal clear.

> Are you saying that the current code (first chunk of code here) is wrong? 
> Otherwise I don't understand how patch v3 is correct.
> 
> Unless the original code is buggy?


Yes, the original(current) code is wrong. Compare the code:

>  uint32_t currentIdleTimeInMS;
>
>  if (NS_FAILED(GetIdleTime(&currentIdleTimeInMS))) {
>    return;
>  }
>
>  if (((PR_Now() - mLastUserInteractionInPR) / PR_USEC_PER_MSEC) >
>      currentIdleTimeInMS)
>  {  
>    ResetIdleTimeOut(currentIdleTimeInMS);
>  }

With an earlier changeset:
http://hg.mozilla.org/mozilla-central/file/87956/widget/xpwidgets/nsIdleService.cpp#l302

>   PRUint32 curTime = static_cast<PRUint32>(PR_Now() / PR_USEC_PER_SEC);
>   PRUint32 lastTime = curTime - mLastHandledActivity;
>
>   // Get the idle time (in seconds).
>   PRUint32 idleTime;
>   if (NS_FAILED(GetIdleTime(&idleTime))) {
>     return;
>   }
> 
>   // GetIdleTime returns the time in ms, internally we only calculate in s.
>   idleTime /= 1000;
>
>   /**
>   * Now, if the idle time, is less than what we expect, it means the
>   * user was active since last time that we checked.
>   */
>
>   bool userActivity = lastTime > idleTime;
>
>   if (userActivity) {
>     if (TryNotifyBackState(idleTime) || idleTime) {
>       RescheduleIdleTimer(idleTime);
>     }
>   }

Which makes more sense of detecting whether we have user activity at a time we checked (taken at time A) and polled time (taken at time B, later than time A), by 

>   bool userActivity = lastTime > idleTime;

in the current code, the time we checked is after the polled time, but still using the same equivalent statement. Thus this is why I think the original(current) code is wrong.

v2 calculates the checked time before the polled time, and does
> if (checkedTime > pulledTime)

v3 calculates the checked time after the polled time, and does
> if (pulledTime > checkedTime)

Which both versions are different then the original one.

Comment 24

[Justin Lebar (not reading bugmail)]

FYI, please don't refer to hg csets with their decimal numbers.  Those numbers aren't necessarily the same across clones.  For example, on my machine, rev 87957 is

$ hg log --rev 87957
changeset:   87957:f6619abc8872
user:        Chris Peterson <[e-mail]>
date:        Mon Feb 27 16:29:44 2012 -0800
summary:     Bug 731056 Part 3 - Fix checkstyle warnings: move field declarations to top of class and make private. r=dougt

which isn't the changeset you're looking for.

Instead, we always refer to hg csets using their hex identifiers, which are unique across clones.

> The original code is buggy. The semantic of the inequality was inverted in the refactoring of r87957

Cool, so the pre-refactoring code matches patch v2 even better; that's great.  But I still don't understand how v3 is correct.  I still feel like I'm missing something key here.

As I understand, we want behavior which matches the pre-refactoring code, which does

> a. lastTime = Now() - mLastHandledActivity;
> b. idleTime = GetIdleTime();
> c. if (lastTime > idleTime);

In contrast, v3 does

> b. idleTime = GetIdleTime();
> a. lastTime = Now() - mLastHandledActivity;
> c. if (idleTime > lastTime)

But why is it correct to go from (a, b, c) to (b, a, c) and flip the inequality?

Let me put it another way: Suppose we run this all within one clock tick, and the value of Now() never changes.  That's a possibility, right?  In that case, it seems to me that the v3 code does something entirely different from the pre-refactoring code.

Comment 25

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

(In reply to Justin Lebar [:jlebar] from comment #24)
> FYI, please don't refer to hg csets with their decimal numbers.  Those
> numbers aren't necessarily the same across clones.

Thanks, I was referring to the numbers on hg.mozilla.org.

> Cool, so the pre-refactoring code matches patch v2 even better; that's
> great.  But I still don't understand how v3 is correct.  I still feel like
> I'm missing something key here.
> 
> As I understand, we want behavior which matches the pre-refactoring code,
> which does
> 
> > a. lastTime = Now() - mLastHandledActivity;
> > b. idleTime = GetIdleTime();
> > c. if (lastTime > idleTime);
> 
> In contrast, v3 does
> 
> > b. idleTime = GetIdleTime();
> > a. lastTime = Now() - mLastHandledActivity;
> > c. if (idleTime > lastTime)

More precisely

 b. idleTime = GetIdleTime();
 e. (user activity updates mLastHandledActivity)
 a. lastTime = Now() - mLastHandledActivity;
 c. if (idleTime >= lastTime)

Since mLastHandledActivity can be changed by other thread, the timing we calculate lastTime will change its value, no matter Now() changes or not. |Now() - mLastHandledActivity| is essentially what we think the user has been idling. So if we replace it with GetIdleTime() it will be obvious

 b. idleTime = GetIdleTime();
 e. (user activity?)
 a. lastTime = GetIdleTime();
 c. if (idleTime >= lastTime)

The difference is that GetIdleTime() is not always equals to |Now() - mLastHandledActivity| because on some platform we query platform idle service, eg. XScreenSaver on X.

BTW, it seems mLastHandledActivity need some r/w protect. It definitely needs more document.

Comment 26

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

I'm not sure if these code still hold its purpose after the refactoring though. The inequality was flipped and nobody noticed it until now.

Comment 27

[Justin Lebar (not reading bugmail)]

> Since mLastHandledActivity can be changed by other thread

Are you sure about that?  This code does not look multithreaded -- as you mention, there are no synchronization primitives anywhere.

ResetIdleTimeOut is the only place where we modify mLastUserInteractionInPR (in the current code).  It runs notifyList[i]->Observe().  In general, observers must run on the main thread.  If ResetIdleTimeOut() were called off the main thread, we'd be calling these observers on whatever thread we happened to be on, which would be a serious bug.  (Not to mention the problems arising from not locking mArrayListeners, etc.)

> The difference is that GetIdleTime() is not always equals to |Now() - 
> mLastHandledActivity| because on some platform we query platform idle service, eg. 
> XScreenSaver on X.

Yes, exactly.  This is the problem.  When GetIdleTime() returns Now() - mLastHandledActivity, v3 is trivially correct.  But what about when it doesn't?  In the case when it doesn't, I don't see how both

> a. lastTime = Now() - mLastHandledActivity;
> b. idleTime = GetIdleTime() = PollIdleTime() = Now() - <system-generated number>
> c. if (lastTime > idleTime);

and

> b. idleTime = GetIdleTime() = PollIdleTime() = Now() - <system-generated number>
> a. lastTime = Now() - mLastHandledActivity;
> d. if (idleTime > lastTime)

can be correct, since, in the simple case when the value of Now() does not change, the comparisons in the last line are not equivalent.

Comment 28

[Shelly Lin [:shelly]]

Attachment: v2: clean up

I see, I finally see the difference, totally miss the case where it is possible that the return value of GetIdleTime() could be not base on mLastHandledActivity.

I've removed the lastIdleTime from v2, but to be corrected to the pre-refactoring code, |now - mLastHandledActivity| has to computed before GetIdleTime().

Comment 29

[Shelly Lin [:shelly]]

Attachment: v2: clean up

Same as the last cancelled attachment, but also correct the comments of checking user activity from IdleTimerCallback().

Comment 30

[Justin Lebar (not reading bugmail)]

Comment on attachment 686958 [details] [diff] [review]
v2: clean up

Okay!  Just one nit and we can check this in and see if it works.  :)

> +  // Find the last detected idle time.
> +  uint32_t lastIdleTimeInMS = (uint32_t)(TimeStamp::Now() -
> +                              mLastUserInteraction).ToMilliseconds();

Nit: This line is wrapped at an awkward place.  We also avoid C-style casts (for reasons I don't totally agree with, but w/e).

One way you could write this is as

  uint32_t lastIdleTimeInMS = static_cast<uint32_t>(
    (TimeStamp::Now() - mLastUserInteraction).ToMilliseconds());

r=me with that nit fixed.  Let's check this in and see if it's any better!

Comment 31

[Gregor Wagner [:gwagner]]

Why is this bug fixed as resolved?

Comment 32

[Gregor Wagner [:gwagner]]

(In reply to Gregor Wagner [:gwagner] from comment #31)
> Why is this bug fixed as resolved?

marked as resolved...

I think we should reopen and update the summary?

Comment 33

[Justin Lebar (not reading bugmail)]

> Why is this bug [marked] as resolved?

It's a source of much confusion.

But this bug does not occur on m-c, because the cset which caused this was backed out.

Feel free to update the metadata if you have a better idea.

Comment 34

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

(In reply to Justin Lebar [:jlebar] from comment #30)
> Comment on attachment 686958 [details] [diff] [review]
> v2: clean up
> 
>   uint32_t lastIdleTimeInMS = static_cast<uint32_t>(
>     (TimeStamp::Now() - mLastUserInteraction).ToMilliseconds());
> 

I prefer constructor syntax for this case

 uint32_t(TimeStamp::Now() - mLastUserInteraction).ToMilliseconds())

cleaner than either alternative and gives much more confidence in the result than casts (measure thrice, cast once).

Comment 35

[Shelly Lin [:shelly]]

Attachment: v2: clean up and carry r+

Clean up and carry r+.

Comment 36

[Shelly Lin [:shelly]]

Try server result:
https://tbpl.mozilla.org/?tree=Try&rev=053833d36d1a
https://tbpl.mozilla.org/?tree=Try&rev=18a0c6b269ba
(The Bi of B2G otoro opt seems not related)

Checkin-needed on bug 803039.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3ef58ba9e55