1038032 - Infinitely repeating animations with a delay don't run

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Rehan Dalal (Inactive)]

Here's a jsfiddle example:
http://jsfiddle.net/D4bbB/1/

Animating the ::before and ::after pseudo elements does not seem to work. This same code worked on previous builds.

Comment 1

[Daniel Holbert [:dholbert]]

Confirmed. In Trunk, I see a single dot appear every five seconds. In Firefox release, I see a column of three dots animate every five seconds.

Bad:
33.0a1 (2014-07-13)
Mozilla/5.0 (X11; Linux x86_64; rv:33.0) Gecko/20100101 Firefox/33.0

Good:
Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0

Hopefully a tighter regression range (and perhaps a more-reduced testcase) will help determine what changed here & why.

Comment 2

[Alice0775 White]

Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9118a2be9b3
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0 ID:20140619194244
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab404d8e7d88
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0 ID:20140619204653
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e9118a2be9b3&tochange=ab404d8e7d88

Regressed by: Bug 1025709

Comment 3

[Alice0775 White]

In local build 
Last Good: 458a988940c6
First Bad: 2d45653b217d

Comment 4

[Brian Birtles (:birtles)]

Thanks Rehan for reporting, and Daniel and Alice for regression tracking!
Taking since this is clearly my fault.

Comment 5

[Brian Birtles (:birtles)]

Turns out this has nothing to do with pseudo-elements and comes down to overflow in TimeDuration. It's actually the :before animation that's animating in the URL from comment 0.

Comment 6

[Brian Birtles (:birtles)]

Attachment: part 1 - Check for integer overflow in TimeDuration operations

Comment 7

[Brian Birtles (:birtles)]

Attachment: part 2 - Add test case for overflowing TimeDuration

Comment 8

[Brian Birtles (:birtles)]

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

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8455923 [details] [diff] [review]
part 1 - Check for integer overflow in TimeDuration operations

># HG changeset patch
># User Brian Birtles <[e-mail]>
># Date 1405400815 -32400
>#      Tue Jul 15 14:06:55 2014 +0900
># Node ID 38fbd3b3bd87a107072a1ab57b5974ca2bc45830
># Parent  2bb155ee786b42bcdee7b4266dc9d32dc8bfc971
>Bug 1038032 part 1 - Check for integer overflow in TimeDuration operations
>
>diff --git a/xpcom/ds/TimeStamp.h b/xpcom/ds/TimeStamp.h
>--- a/xpcom/ds/TimeStamp.h
>+++ b/xpcom/ds/TimeStamp.h
>@@ -80,69 +80,81 @@ public:
> 
>   static TimeDuration Forever()
>   {
>     return FromTicks(INT64_MAX);
>   }
> 
>   TimeDuration operator+(const TimeDuration& aOther) const
>   {
>-    return TimeDuration::FromTicks(mValue + aOther.mValue);
>+    if (static_cast<double>(mValue) + aOther.mValue > INT64_MAX) {
>+      return Forever();
>+    }
>+    return FromTicks(mValue + aOther.mValue);

Drive-by comment on part 1: I'm not sure this check is appropriate for all clients.  We could, hypothetically, do this sort of check for all of our integer-backed types (e.g. nscoord, IntRect), but we generally don't, because we don't want to take the overhead of the check everywhere, and we instead add checks (or wrapper-functions like NSCoordSaturating*) where we explicitly know we might be dealing with infinite/near-infinite values.

So, 2 questions:
 1) Can we lift this checking out of ::operator+ and into either a custom "saturating addition" wrapper-function? 
 2) In that wrapper-function, is it sufficient to simply check if either operand is ::Forever(), and return ::Forever() if so?

>   }
>   TimeDuration operator-(const TimeDuration& aOther) const
>   {
>-    return TimeDuration::FromTicks(mValue - aOther.mValue);
>+    if (static_cast<double>(mValue) - aOther.mValue < INT64_MIN) {
>+      return FromTicks(INT64_MIN);
>+    }
>+    return FromTicks(mValue - aOther.mValue);
>   }

(I'm hoping that we don't depend on "operator-" doing the right thing with ::Forever()-ish values. Might be worth adding a NS_WARNING that neither value is ::Forever() there.  (And if we need it, maybe add a wrapper function like the one suggested above, which checks if the number being subtracted from is ::Forever(), and if so, return ::Forever? And call that wrapper-function from any animation code that might expect to be subtracting from infinity.)

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8455924 [details] [diff] [review]
part 2 - Add test case for overflowing TimeDuration

r=me on the testcase, assuming it fails pre-patch and passes post-patch.

(Do we need more testcases to exercise the various ::operator-(), MultiplyValueByInteger, etc. code-paths in the fix? I'm really hoping that most of the bounds-checks there are actually sorta unnecessary, per previous comment, but if they are necessary, we should be sure there's test coverage for them.)

Comment 11

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #9)
> Comment on attachment 8455923 [details] [diff] [review]
> part 1 - Check for integer overflow in TimeDuration operations
> 
> ># HG changeset patch
> ># User Brian Birtles <[e-mail]>
> ># Date 1405400815 -32400
> >#      Tue Jul 15 14:06:55 2014 +0900
> ># Node ID 38fbd3b3bd87a107072a1ab57b5974ca2bc45830
> ># Parent  2bb155ee786b42bcdee7b4266dc9d32dc8bfc971
> >Bug 1038032 part 1 - Check for integer overflow in TimeDuration operations
> >
> >diff --git a/xpcom/ds/TimeStamp.h b/xpcom/ds/TimeStamp.h
> >--- a/xpcom/ds/TimeStamp.h
> >+++ b/xpcom/ds/TimeStamp.h
> >@@ -80,69 +80,81 @@ public:
> > 
> >   static TimeDuration Forever()
> >   {
> >     return FromTicks(INT64_MAX);
> >   }
> > 
> >   TimeDuration operator+(const TimeDuration& aOther) const
> >   {
> >-    return TimeDuration::FromTicks(mValue + aOther.mValue);
> >+    if (static_cast<double>(mValue) + aOther.mValue > INT64_MAX) {
> >+      return Forever();
> >+    }
> >+    return FromTicks(mValue + aOther.mValue);
> 
> Drive-by comment on part 1: I'm not sure this check is appropriate for all
> clients.  We could, hypothetically, do this sort of check for all of our
> integer-backed types (e.g. nscoord, IntRect), but we generally don't,
> because we don't want to take the overhead of the check everywhere, and we
> instead add checks (or wrapper-functions like NSCoordSaturating*) where we
> explicitly know we might be dealing with infinite/near-infinite values.

That seems reasonable. I haven't looked into how to go about it yet but I'm also concerned about the overhead these checks add. I'd suggest that Forever() should *only* be defined on this wrapper type.

> So, 2 questions:
>  1) Can we lift this checking out of ::operator+ and into either a custom
> "saturating addition" wrapper-function? 

I think it's worth trying.

>  2) In that wrapper-function, is it sufficient to simply check if either
> operand is ::Forever(), and return ::Forever() if so?

I'm less sure about this. Maybe though.

> >   }
> >   TimeDuration operator-(const TimeDuration& aOther) const
> >   {
> >-    return TimeDuration::FromTicks(mValue - aOther.mValue);
> >+    if (static_cast<double>(mValue) - aOther.mValue < INT64_MIN) {
> >+      return FromTicks(INT64_MIN);
> >+    }
> >+    return FromTicks(mValue - aOther.mValue);
> >   }
> 
> (I'm hoping that we don't depend on "operator-" doing the right thing with
> ::Forever()-ish values. Might be worth adding a NS_WARNING that neither
> value is ::Forever() there.  (And if we need it, maybe add a wrapper
> function like the one suggested above, which checks if the number being
> subtracted from is ::Forever(), and if so, return ::Forever? And call that
> wrapper-function from any animation code that might expect to be subtracting
> from infinity.)

That's a good point. I was mostly looking at avoiding wrapping at the limits but it seems like we should always treat a "Forever" manner more like IEEE-754 Infinity and, like you say, make Forever - a = Forever.

That sounds like a special kind of behavior that probably deserves a wrapper.

Comment 12

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> Comment on attachment 8455924 [details] [diff] [review]
> part 2 - Add test case for overflowing TimeDuration
> 
> r=me on the testcase, assuming it fails pre-patch and passes post-patch.
> 
> (Do we need more testcases to exercise the various ::operator-(),
> MultiplyValueByInteger, etc. code-paths in the fix? I'm really hoping that
> most of the bounds-checks there are actually sorta unnecessary, per previous
> comment, but if they are necessary, we should be sure there's test coverage
> for them.)

I thought about this but I decided it's too much an indirect way of testing. If we change the order in which timing parameters are added inside the animations code, suddenly we'd lose test coverage of TimeStamp without realising it. I noticed we have TestTimeStamp.cpp but it doesn't seem to be used.

Comment 13

[Brian Birtles (:birtles)]

Comment on attachment 8455923 [details] [diff] [review]
part 1 - Check for integer overflow in TimeDuration operations

Cancelling the review request for now since I think it's probably worth looking into a wrapper class.

Comment 14

[Brian Birtles (:birtles)]

Attachment: WIP patch to create SaturatingTimeDuration wrapper

Here is a WIP patch at a wrapper (SaturatingTimeDuration) that checks if either
argument to the arithmetic operation is Forever and maintains the Forever-ness
if so.

I've implemented it using a templated class with a ValueCalculator argument.
I tried making a regular wrapper but keeping the method signatures on both
wrapper and wrappee synchronized seemed like a maintenance headache. Also, I'd
like to avoid adding virtual functions so I didn't try inheritance.

The current arrangement lets us keep TimeDuration with its existing behavior and
adds SaturatingTimeDuration for when we expect the duration to contain Forever
values (and we also expect it to be involved in arithmetic operations).

I added a ctor to convert from one type to the other but marked it as explicit
so you have to actually write "TimeDuration(saturatingDur)". Hopefully that
makes hard to accidentally skip overflow checking due to implicit conversions.

This patch also adds a few little tweaks like converting between Forever and
Infinity when converting from int<->double.

I've split off the platform-specific code into a separate class so it doesn't
have to deal with templates and and so we can add some cross-platform checks to
the calling of these methods (like the Forever<->Infinity conversion mentioned
above).

If you think this approach makes sense I'll split it up into several small
steps.

Fair to say, though, we're not going to land anything this ambitious before the
next uplift so I'll need to prepare a more localized workaround for the
regression identified in this bug.

Comment 15

[Brian Birtles (:birtles)]

I forgot to mention, this patch (the WIP patch) only works for Windows. Updating the other platforms is trivial.

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8456656 [details] [diff] [review]
WIP patch to create SaturatingTimeDuration wrapper

I think this makes sense, at a high level.

For variables that could end up being infinite (like representations of the active duration), we need to make sure that the arithmetic we perform can handle ::Forever() in a sane way.  I'd been imagining using special functions, specifically on these variables, but using a special type with operators seems reasonable too.

Having said that, I think this struct does seem a bit "viral". :)  Ideally, I think a Saturating type for variables that can *actually* be ::Forever() (and even then, maybe only when we're preparing to do addition/subtraction with those variables).

With those restrictions (and  I suspect (?) a lot of the changes in this patch become unnecessary.

For example:

>diff --git a/layout/style/AnimationCommon.cpp b/layout/style/AnimationCommon.cpp
> ComputedTiming
> ElementAnimation::GetComputedTimingAt(TimeDuration aLocalTime,
>                                       const AnimationTiming& aTiming)
> {
>-  const TimeDuration zeroDuration;
>+  const SaturatingTimeDuration zeroDuration;
[...]
>   MOZ_ASSERT(aTiming.mIterationDuration >= zeroDuration,
>              "Expecting iteration duration >= 0");

For example: this seems a bit odd. The variable zeroDuration is clearly not at risk of containing ::Forever(), and it clearly won't participate in any addition in a way that has a chance of breaking any ::Forever values. It looks like it's just being converted so that it can be compared to another SaturatingTimeDuration value.  I'd rather we don't have to do this sort of thing, or else eventually most TimeDurations will have been replaced with a SaturatingTimeDuration. :)

As noted below, I think this other value probably wants to stay TimeDuration. However, even if it didn't, I think this could be cleaner (and less "viral") if we added a conversion operator or a comparison operator or something like that. (We may not need those, though, since at least in this case, I'm hoping that neither of these variables needs to be Saturating.)

>diff --git a/layout/style/AnimationCommon.h b/layout/style/AnimationCommon.h
>@@ -240,18 +241,18 @@ struct AnimationProperty
>  * Input timing parameters.
>  *
>  * Eventually this will represent all the input timing parameters specified
>  * by content but for now it encapsulates just the subset of those
>  * parameters passed to GetPositionInIteration.
>  */
> struct AnimationTiming
> {
>-  mozilla::TimeDuration mIterationDuration;
>-  mozilla::TimeDuration mDelay;
>+  mozilla::SaturatingTimeDuration mIterationDuration;
>+  mozilla::SaturatingTimeDuration mDelay;
>   float mIterationCount; // mozilla::PositiveInfinity<float>() means infinite

Similarly, I don't think either of these can actually be ::Forever(), can they?  Of course, if the iteration count is infinity, then ::ActiveDuration() could be ::Forever().  But I don't think *these* can.

If I'm not mistaken, then I think I'd prefer that these stay TimeDuration, and either:
 (1) Make ActiveDuration() return SaturatingTimeDuration (as you have it doing) and manage the conversion from TimeDuration to SaturatingTimeDuration
 (2) Make ActiveDuration() keep returning a TimeDuration, with documentation updated to state that it can return ::Forever(), and then the caller (aware of the ::Forever() possibility) can choose to wrap that in a SaturatingTimeDuration or not as they see fit.
...or:
 (3) Like (2), but instead of callers wrapping the result in a SaturatingTimeDuration(), callers just invoke a Saturating addition/subtraction function that operates on TimeDuration().

I'm pretty sure I prefer (2) or (3) over (1) (and over the current patch), since they're less "viral" -- that is, if they seem workable to you.

Comment 17

[Daniel Holbert [:dholbert]]

Sorry, insufficient editing:
(In reply to Daniel Holbert [:dholbert] from comment #16)
> Having said that, I think this struct does seem a bit "viral". :)  Ideally,
> I think a Saturating type for variables that can *actually* be ::Forever()

I dropped a few words there -- I meant:
 "I think _we should only use_ a Saturating type"

> With those restrictions (and  I suspect (?) a lot of the changes in this
> patch become unnecessary.

I forgot to edit out the "(and" there. Ignore it. :)

Comment 18

[Daniel Holbert [:dholbert]]

(My preferred options from comment 16, labeled (2) and (3), do require more consideration in the callers, and careful thought around any arithmetic that involves TimeDuration values RE whether there's any possibility that they could *meaningfully* be ::Forever().  I think this cost is probably reasonable, and is on balance saner than having to use SaturatingTimeDuration semi-arbitrarily on known-not-to-be-Forever() time values.)

(RE "*meaningfully*" -- I think we can worry less about situations that happen to produce ::Forever()-ish values due to absurdly large input.  For example, "animation-iteration-count: infinite;" would produce a ::Forever() active duration in a *meaningful* way, and we need to handle that correctly one way or another. Whereas, e.g. "animation-delay: 999999999999999999999s" might conceivably produce ::Forever() in mDelay, but we don't necessarily need to special-case that possibility, as long as we're not going to hang or crash.)

Comment 19

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #16)
> Comment on attachment 8456656 [details] [diff] [review]
> WIP patch to create SaturatingTimeDuration wrapper
> 
> I think this makes sense, at a high level.
> 
> For variables that could end up being infinite (like representations of the
> active duration), we need to make sure that the arithmetic we perform can
> handle ::Forever() in a sane way.  I'd been imagining using special
> functions, specifically on these variables, but using a special type with
> operators seems reasonable too.

I was probably subconsciously influenced by the current proposal for NSCoord was to have a saturating type rather than saturating operations (bug 575011). It seems we learnt there that saturating operations are not very maintainable and a new type is preferable.

> Having said that, I think this struct does seem a bit "viral". :)  Ideally,
> I think a Saturating type for variables that can *actually* be ::Forever()
> (and even then, maybe only when we're preparing to do addition/subtraction
> with those variables).

I think that's partly because I was a bit eager in converting types. See below, but basically some of the places can remain TimeDuration. Comparison operations, for a start, work across different templated types and an explicit ctor converts between templated types when we come to do arithmetic.

(In reply to Daniel Holbert [:dholbert] from comment #16)
> >diff --git a/layout/style/AnimationCommon.cpp b/layout/style/AnimationCommon.cpp
> > ComputedTiming
> > ElementAnimation::GetComputedTimingAt(TimeDuration aLocalTime,
> >                                       const AnimationTiming& aTiming)
> > {
> >-  const TimeDuration zeroDuration;
> >+  const SaturatingTimeDuration zeroDuration;
> [...]
> >   MOZ_ASSERT(aTiming.mIterationDuration >= zeroDuration,
> >              "Expecting iteration duration >= 0");
> 
> For example: this seems a bit odd. The variable zeroDuration is clearly not
> at risk of containing ::Forever(), and it clearly won't participate in any
> addition in a way that has a chance of breaking any ::Forever values. It
> looks like it's just being converted so that it can be compared to another
> SaturatingTimeDuration value.  I'd rather we don't have to do this sort of
> thing, or else eventually most TimeDurations will have been replaced with a
> SaturatingTimeDuration. :)

We don't actually need to change the type of zeroDuration, that change was a search and replace bug. The template handles comparison between different templated types so zeroDuration can remain TimeDuration and be compared as-is.

> As noted below, I think this other value probably wants to stay
> TimeDuration. However, even if it didn't, I think this could be cleaner (and
> less "viral") if we added a conversion operator or a comparison operator or
> something like that. (We may not need those, though, since at least in this
> case, I'm hoping that neither of these variables needs to be Saturating.)

We have those already. The conversion operator is the constructor which converts between different templated types. There is deliberately no implicit conversion operator (and the ctor is marked as explicit) so that you don't end up silently falling back to non-saturating arithmetic when one argument is not of the correct type. Instead you have to explicitly convert one argument to either Saturating or not depending on what type of arithmetic you want in that case.

Alternatively, we could define some template specializations for, e.g. operator+ to handle the case when one argument is saturating and one is not and which automatically invoke the saturating operation. That way we'd always fall back to a saturating operation if one argument was saturating without having to do explicit conversion.

> >diff --git a/layout/style/AnimationCommon.h b/layout/style/AnimationCommon.h
> >@@ -240,18 +241,18 @@ struct AnimationProperty
> >  * Input timing parameters.
> >  *
> >  * Eventually this will represent all the input timing parameters specified
> >  * by content but for now it encapsulates just the subset of those
> >  * parameters passed to GetPositionInIteration.
> >  */
> > struct AnimationTiming
> > {
> >-  mozilla::TimeDuration mIterationDuration;
> >-  mozilla::TimeDuration mDelay;
> >+  mozilla::SaturatingTimeDuration mIterationDuration;
> >+  mozilla::SaturatingTimeDuration mDelay;
> >   float mIterationCount; // mozilla::PositiveInfinity<float>() means infinite
> 
> Similarly, I don't think either of these can actually be ::Forever(), can
> they?  Of course, if the iteration count is infinity, then
> ::ActiveDuration() could be ::Forever().  But I don't think *these* can.

Yes, so long as we're only supporting CSS these won't be Forever. In the Web Animations model however, mIterationDuration can be Forever. mDelay currently can't but probably will be able to be in the future. So we could leave these as TimeDuration for now.

> If I'm not mistaken, then I think I'd prefer that these stay TimeDuration,
> and either:
>  (1) Make ActiveDuration() return SaturatingTimeDuration (as you have it
> doing) and manage the conversion from TimeDuration to SaturatingTimeDuration
>  (2) Make ActiveDuration() keep returning a TimeDuration, with documentation
> updated to state that it can return ::Forever(), and then the caller (aware
> of the ::Forever() possibility) can choose to wrap that in a
> SaturatingTimeDuration or not as they see fit.
> ...or:
>  (3) Like (2), but instead of callers wrapping the result in a
> SaturatingTimeDuration(), callers just invoke a Saturating
> addition/subtraction function that operates on TimeDuration().
> 
> I'm pretty sure I prefer (2) or (3) over (1) (and over the current patch),
> since they're less "viral" -- that is, if they seem workable to you.

I don't like (2) because it seems highly likely to me that some call site of ActiveDuration will get updated without referring to the documentation, forget to convert to a saturating time duration, and produce a bug like this one.

Also, having seen the comments in bug 575011, I'm a little wary of (3).

Comment 20

[Brian Birtles (:birtles)]

Attachment: part 1 - Add temporary workaround to avoid overflow when calculating the active end of an animation

Here's a minimal patch to fix the regression before the next uplift. I've split off bug 1039924 for doing a more thorough job.

Comment 21

[Daniel Holbert [:dholbert]]

(In reply to Brian Birtles (:birtles) from comment #19)
> There is deliberately no
> implicit conversion operator (and the ctor is marked as explicit) so that
> you don't end up silently falling back to non-saturating arithmetic when one
> argument is not of the correct type. Instead you have to explicitly convert

This sounds good to me.

> Alternatively, we could define some template specializations for, e.g.
> operator+ to handle the case when one argument is saturating and one is not
> and which automatically invoke the saturating operation. That way we'd
> always fall back to a saturating operation if one argument was saturating
> without having to do explicit conversion.

This also sounds fine. :)

> > Similarly, I don't think either of these can actually be ::Forever(), can
> > they?  Of course, if the iteration count is infinity, then
> > ::ActiveDuration() could be ::Forever().  But I don't think *these* can.
> 
> Yes, so long as we're only supporting CSS these won't be Forever. In the Web
> Animations model however, mIterationDuration can be Forever. mDelay
> currently can't but probably will be able to be in the future. So we could
> leave these as TimeDuration for now.

OK -- yeah, maybe these should stay TimeDuration for now, with a comment about how they may need upgrading in the future.

> I don't like (2) because it seems highly likely to me that some call site of
> ActiveDuration will get updated without referring to the documentation,
> forget to convert to a saturating time duration, and produce a bug like this
> one.
> 
> Also, having seen the comments in bug 575011, I'm a little wary of (3).

Good points.

I'd forgotten about bug 575011.  One note on that, though -- the problems described there don't fully apply here.  It looks like the main gripe (or one of the main gripes, at least) in that bug is about the spammy assertions for out-of-range nscoords being (outside the [nscoord_MIN, nscoord_MAX] range), which can happen if we forget to use saturating math, or if we just have content with huge sizes.  Those sort of assertions probably wouldn't exist here (since TimeDuration doesn't explicitly declare half of its numeric range to be invalid, like nscoord does). So we probably don't have to worry about that.

Setting that aside, though: would you be ok with something closer to option (1), then? That's grown on me a bit, after reading your comment here and re-skimming bug 575011.

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 8457782 [details] [diff] [review]
part 1 - Add temporary workaround to avoid overflow when calculating the active end of an animation

r=me.

(If you discover other places that are similarly susceptible to this problem (and are recent regressions) when fixing bug 1039924, we'll probably want to backport targeted fixes for those, too.)

Comment 23

[Brian Birtles (:birtles)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/39de47827a38
https://hg.mozilla.org/integration/mozilla-inbound/rev/14c7a4ee44fc

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/39de47827a38
https://hg.mozilla.org/mozilla-central/rev/14c7a4ee44fc