Closed Bug 1380133 Opened 7 years ago Closed 7 years ago

stylo: HackerRank menu hover animation flickers

Categories

(Core :: CSS Parsing and Computation, enhancement, P2)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: jdm, Assigned: heycam)

References

(Blocks 1 open bug)

Details

Attachments

(12 files, 5 obsolete files)

Testcase
287 bytes, text/html
Details
Mochitest
2.21 KB, patch
Details | Diff | Splinter Review
Bug 1380133 - Part 1: Minor reformatting and encapsulation.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1380133 - Part 2: Take a ServoStyleContext* in Gecko_CalcStyleDifference.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1380133 - Part 3: Remove nsPresContext argument to ServoStyleContext::ResolveSameStructsAs.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1380133 - Part 4: Make CalcStyleDifferenceInternal not cache any new structs on ServoStyleContexts when in a traversal.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1380133 - Part 5: Call CalcStyleDifference with ServoStyleContexts instead of a FakeStyleContext wrapping a ServoComputedValues.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1380133 - Part 6: Remove FakeStyleContext.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1380133 - Part 7: De-templatize CalcStyleDifference(Internal).
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1380133 - Part 8: Update test expectations.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1380133 - Part 9: Automated test for inheriting the start value of a transition.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1380133 - Part 10: Whitelist nsStyleContext::PeekStyleXXX for heap writes.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
As a hiring manager, visiting https://www.hackerrank.com/x/tests shows me a menu panel on the left side of the screen. When running with stylo disabled, the hover transition is a smooth highlight. With stylo enabled, the hover transition flickers rapidly before reaching full opacity.
Attached file Testcase — 
This is a minimal testcase. If you remove the spans, or if you remove the non-color transition, the problem does not manifest.
Attachment #8885439 - Attachment mime type: text/plain → text/html
This might be also related to animation-only restyle triggered by event handling.
Component: Layout → CSS Parsing and Computation
This is not related to event handling, it happens even if DoProcessPendingRestyles is not called in ServoRestyleManager::UpdateOnlyAnimationStyles().
Priority: -- → P2
(In reply to Josh Matthews [:jdm] from comment #1)
> This is a minimal testcase. If you remove the spans, or if you remove the
> non-color transition, the problem does not manifest.

I see the problem even without the non-color transition. Specifically it seems like we do an initial paint with the transition end color and then remove it and do the transition from the start.

I wonder if we're failing to correctly inherit the color applied during the subsequent animation-only restyle.
I had a look into this and I think I worked out what is going on here, although I'm not sure how it is supposed to work.

On :hover we update the 'color' on the <a> element from #979faf to #f8f9fa. Incidentally, at this point we correctly calculate the style difference and decide we MustCascadeChildren so we update the 'color' on the child <span> to #f8f9fa as well. So far, so good.

Then, in the sequential task following the parallel traversal we run Gecko_UpdateAnimations and generate a corresponding transition.

Since this is a normal restyle we call PreTraverse to see if we need to do a second traversal and discover that we do (since we generated a new transition). We start the new traversal from the root <html> element.

We begin with the animation-only restyle.

At this point the relevant part of the subtree data looks like the following:

>  <li> (0x1f0b4ed8b30) dd=true data=Some(ElementData { styles: ElementStyles { primary: Some(ComputedValues { rules: Some(StrongRuleNode { p: 0x1f0b324c1f0 }), .. }), pseudos: EagerPseudoStyles(None) }, restyle: RestyleData { hint: , flags: , damage: GeckoRestyleDamage(nsChangeHint(0)) } })
>    <a> (0x1f0b4e85380) dd=true data=Some(ElementData { styles: ElementStyles { primary: Some(ComputedValues { rules: Some(StrongRuleNode { p: 0x1f0b3142060 }), .. }), pseudos: EagerPseudoStyles(None) }, restyle: RestyleData { hint: RESTYLE_CSS_TRANSITIONS, flags: WAS_RESTYLED, damage: GeckoRestyleDamage(nsChangeHint(1)) } })
>      <span> (0x1f0b4ed8bc0) dd=false data=Some(ElementData { styles: ElementStyles { primary: Some(ComputedValues { rules: Some(StrongRuleNode { p: 0x1f0b54fb150 }), .. }), pseudos: EagerPseudoStyles(None) }, restyle: RestyleData { hint: , flags: WAS_RESTYLED, damage: GeckoRestyleDamage(nsChangeHint(1)) } })

We go to restyle the <a> element with RestyleKind CascadeWithReplacements(RESTYLE_CSS_TRANSITIONS).

At this point we will generate and replace the transition rule which will set 'color' to #979faf (i.e. the starting point of the transition which happens to coincide with the original style of the element before applying the :hover style).

After that when we call finish_restyle on the TElement, the ElementData will have a value of #f8f9fa (i.e. the transition end point, nscolor: 4294638072). We store that as old_styles and update it with the new value of #979faf (i.e. the transition start point, nscolor: 4289699735).

Then we will pass these styles to accumulate_damage_for. Even here |old_values| and |new_values| correctly represent the old and new values that result from the animation-only restyle, i.e. #f8f9fa and #979faf respectively.

We pass these on to compute_style_difference which does:

    if let Some(source) = self.existing_style_for_restyle_damage(old_values, pseudo) {
        return RestyleDamage::compute_style_difference(source, new_values)
    }

In existing_style_for_restyle_damage we call Gecko_GetStyleContext which grabs the nsStyleContext from the element's frame and returns that. However, at this point we haven't updated the style context on the frame because we haven't run the post traversal steps yet. As a result the nsStyleContext still has a computed 'color' of #979faf (i.e. the value before we applied the :hover style, which coincides with the transition start point).

Instead of using |old_values| to calculate the style difference (which would give us the correct result), we end up passing this old nsStyleContext to Gecko_CalcStyleDifference which (correctly) determines that nothing has changed since both the old style context and our updated computed values both have a 'color' value of #979faf.

In effect we have the following sequence:

   1. Original style, #979faf
                |
                v
   2. Style after applying :hover, #f8f9fa
                |
                v
   3. Style after applying start of transition, #979faf

and we end up comparing (1) and (3) and deciding nothing has changed.

compute_style_difference then returns GeckoRestyleDamage(nsChangeHint(0)) / StyleChange::Unchanged. As a result, accumulate_damage_for returns ChildCascadeRequirement::CanSkipCascade and we never go to restyle the child <span> element leaving it with the 'color' it inherited at step (2) above: #f8f9fa. Hence the flicker. We'll paint at least one frame with the *end* point of the transition before jumping back to (roughly) the start of the transition again.
OK, so here is a question.  Why do have existing_style_for_restyle_damage, rather than using the ComputedValues in the ElementData?  I assume it's primarily because we need to use the old nsStyleContext so we can look at its mBits to determine which structs to generate damage for, and which can be ignored for damage.  We could make nsStyleContext::CalcStyleDifferenceInternal work on a ComputedValues object for the old style, as long as we provided it with accurate mBits value, and we could use the old old nsStyleContext's mBits (since I think they should be the same as the what the not-yet-created newer old nsStyleContext's would be).

CalcStyleDifferenceInternal also does some visited style difference computation, but as pointed out in the comment on FakeStyleContext::GetStyleIfVisited we don't handle that correctly for bare ComputedValues.

Also, I'm not sure how Manish's work affects this.

Emilio, do you think it makes sense to do CalcStyleDifference on two ComputedValues objects here?
Flags: needinfo?(emilio+bugs)
I'll take a quick shot at that on the assumption it's sensible.
Assignee: nobody → cam
Status: NEW → ASSIGNED
It makes sense, over all. In particular, note bug 1289622.

The reason to require an nsStyleContext was primarily the used structs optimization... I guess we can always rip it off and generate changes for all the structs.
Flags: needinfo?(emilio+bugs)
See Also: → 1289622
Attached patch Mochitest — — Splinter Review 
Here's a mochitest for you. Fails for me with Stylo enabled, passes with it disabled.
Thanks for the test, Brian.  It passes with this WIP patch, though I haven't done extensive other testing yet.

Also I'm pretty sure bug 1367904 will bitrot me horribly, so I might wait until that lands before getting this into shape for review.
Err, the bug I wanted to point to wasn't that one, but the one that bz opened with a similar patch to allow diffing two ServoComputedValues... But I can't find that apparently.
OK, hopefully I haven't duplicated too much work then...
Attachment #8886925 - Attachment is obsolete: true
Attachment #8886926 - Attachment is obsolete: true
Attachment #8886927 - Attachment is obsolete: true
Attachment #8886917 - Flags: review?(emilio+bugs)
Attachment #8886921 - Flags: review?(emilio+bugs)
Comment on attachment 8886751 [details]
Bug 1380133 - Part 1: Minor reformatting and encapsulation.

https://reviewboard.mozilla.org/r/157562/#review164426

::: layout/style/ServoStyleContext.h:28
(Diff revision 4)
>                      nsIAtom* aPseudoTag,
>                      CSSPseudoElementType aPseudoType,
>                      ServoComputedValuesForgotten aComputedValues);
>  
> -  nsPresContext* PresContext() const {
> -    return mPresContext;
> +  nsPresContext* PresContext() const { return mPresContext; }
> +  const ServoComputedValues* ComputedValues() const { return &mSource; }

nit: This should probably return a const reference, but given the change would be less trivial, not going to insist too much :)

::: layout/style/ServoStyleContext.h:30
(Diff revision 4)
>  
> -  nsPresContext* PresContext() const {
> -    return mPresContext;
> +  nsPresContext* PresContext() const { return mPresContext; }
> +  const ServoComputedValues* ComputedValues() const { return &mSource; }
> -  }
>  
> -  const ServoComputedValues* ComputedValues() const {
> +  void AddRef() { Servo_StyleContext_AddRef(this); } 

nit: trailing whitespace.

::: layout/style/ServoTypes.h:198
(Diff revision 4)
> - * We want C++ to be abe to read the style struct fields of ComputedValues
> + * We want C++ to be able to read the style struct fields of ComputedValues
>   * so we define this type on the C++ side and use the bindgenned version
>   * on the Rust side.
> - *
>   */
> -struct ServoComputedValues {
> +class ServoComputedValues

So, this makes this a class with different access specifiers, which probably make this type not an standard layout type[1].

But we're abusing the ABI representation everywhere else, so I suppose it's fine, and this is actually nicer.

[1]: http://en.cppreference.com/w/cpp/concept/StandardLayoutType

::: layout/style/ServoTypes.h:203
(Diff revision 4)
> -struct ServoComputedValues {
> +class ServoComputedValues
> +{
> +  friend class mozilla::ServoStyleContext;
> +
> +public:
> +  // Constructs via memcpy. Will not invalidate old struct

nit: not sure what "invalidate" is supposed to mean in this context... perhaps s/invalidate/destroy/?

::: layout/style/nsAnimationManager.h:21
(Diff revision 4)
>  #include "nsRFPService.h"
>  
>  class nsIGlobalObject;
>  class nsStyleContext;
>  struct nsStyleDisplay;
> -struct ServoComputedValues;
> +class ServoComputedValues;

nit: Not sure if what's the exact policy to sort the forward declarations, depends if you count the `class`/`struct` prefix.

If you know and this is supposed to be after `nsStyleContext`, please update, otherwise just disregard this comment entirely :)
Attachment #8886751 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8886917 [details]
style: Pass a &ComputedValues (aka ServoStyleContext*) to Gecko_CalcStyleDifference.

https://reviewboard.mozilla.org/r/157658/#review164428
Attachment #8886917 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8886918 [details]
Bug 1380133 - Part 2: Take a ServoStyleContext* in Gecko_CalcStyleDifference.

https://reviewboard.mozilla.org/r/157660/#review164432

::: layout/style/ServoBindings.h:381
(Diff revision 3)
>  // Also, we might want a ComputedValues to ComputedValues API for animations?
>  // Not if we do them in Gecko...
>  nsStyleContext* Gecko_GetStyleContext(RawGeckoElementBorrowed element,
>                                        nsIAtom* aPseudoTagOrNull);
>  mozilla::CSSPseudoElementType Gecko_GetImplementedPseudo(RawGeckoElementBorrowed element);
> -nsChangeHint Gecko_CalcStyleDifference(nsStyleContext* oldstyle,
> +nsChangeHint Gecko_CalcStyleDifference(nsStyleContext* old_style_from_frame,

Probably the left one should also be const, if while you're at it you could make the changes, it'd be awesome :)
Attachment #8886918 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8886919 [details]
Bug 1380133 - Part 3: Remove nsPresContext argument to ServoStyleContext::ResolveSameStructsAs.

https://reviewboard.mozilla.org/r/157662/#review164436
Attachment #8886919 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8886920 [details]
Bug 1380133 - Part 4: Make CalcStyleDifferenceInternal not cache any new structs on ServoStyleContexts when in a traversal.

https://reviewboard.mozilla.org/r/157664/#review164438

::: layout/style/nsStyleContext.cpp:262
(Diff revision 3)
>      } else {                                                                  \
>        unrequestedStruct = false;                                              \
>      }                                                                         \
>      if (this##struct_) {                                                      \
> -      const nsStyle##struct_* other##struct_ = aNewContext->Style##struct_(); \
> +      const nsStyle##struct_* other##struct_ =                                \
> +        aNewContext->ThreadsafeStyle##struct_();                              \

FWIW I find these ThreadsafeFoo's methods behaviour a little bit too magic...
Attachment #8886920 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8886920 [details]
Bug 1380133 - Part 4: Make CalcStyleDifferenceInternal not cache any new structs on ServoStyleContexts when in a traversal.

https://reviewboard.mozilla.org/r/157664/#review164440

::: commit-message-8c52d:1
(Diff revision 3)
> +Bug 1380133 - Part 4: Make CalcStyleDifferenceInternal never cache new structs on ServoStyleContexts. r?emilio

Probably this should include something about "during the parallel traversal", or something like that.
Comment on attachment 8886921 [details]
style: Pass old ComputedValues and old cached struct bits to Gecko_CalcStyleDifference.

https://reviewboard.mozilla.org/r/157666/#review164442

::: servo/components/style/gecko/restyle_damage.rs:52
(Diff revision 3)
>      /// structs, so they effectively only diff structs that have ever been
>      /// accessed from layout.
>      pub fn compute_style_difference(
>          source: &nsStyleContext,
> -        new_style: &Arc<ComputedValues>
> +        old_style: &ComputedValues,
> +        new_style: &Arc<ComputedValues>,

I think now we can convert this to `&ComputedValues`, mind filing a followup/servo E-easy issue once this lands?
Attachment #8886921 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8886922 [details]
Bug 1380133 - Part 5: Call CalcStyleDifference with ServoStyleContexts instead of a FakeStyleContext wrapping a ServoComputedValues.

https://reviewboard.mozilla.org/r/157668/#review164444

::: layout/style/ServoBindings.cpp:410
(Diff revision 3)
>  {
> -  MOZ_ASSERT(aOldStyleFromFrame);
> +  MOZ_ASSERT(aOldStyle);
>    MOZ_ASSERT(aNewStyle);
>  
> -  uint32_t equalStructs, samePointerStructs;
> -  nsChangeHint result =
> +  uint32_t relevantStructs =
> +    static_cast<uint32_t>(aOldStyleBits & NS_STYLE_INHERIT_MASK);

nit: Do you really need/want the static_cast?

::: layout/style/ServoBindings.cpp:412
(Diff revision 3)
>    MOZ_ASSERT(aNewStyle);
>  
> -  uint32_t equalStructs, samePointerStructs;
> -  nsChangeHint result =
> -    aOldStyleFromFrame->CalcStyleDifference(aNewStyle->ComputedValues(),
> +  uint32_t relevantStructs =
> +    static_cast<uint32_t>(aOldStyleBits & NS_STYLE_INHERIT_MASK);
> +
> +  uint32_t equalStructs, samePointerStructs;  // unused

Well, not quite unused, at least `equalStructs`, I guess...

::: layout/style/nsStyleContext.h:219
(Diff revision 3)
>    inline nsRuleNode* RuleNode();
>    inline const ServoComputedValues* ComputedValues();
>  
>    void AddStyleBit(const uint64_t& aBit) { mBits |= aBit; }
>  
> +  bool OnlyHasCachedStructs(uint64_t aStructs)

nit: const

::: layout/style/nsStyleContext.h:264
(Diff revision 3)
>    #include "nsStyleStructList.h"
>    #undef STYLE_STRUCT
>  
> +  // Value that can be passed as CalcStyleDifference's aRelevantStructs
> +  // argument to indicate that all structs that are currently resolved on the
> +  // old style context should be compared.

Document that this is only relevant for Servo style contexts?

(Actually I wonder if `CalcStyleDifferenceInternal` should just be a static function and all these branches could/should go away)

::: layout/style/nsStyleContext.h:282
(Diff revision 3)
>     * siblings.  Most (all of those except the "NotHandledForDescendants"
>     * hints) also apply to all descendants.
>     *
>     * aEqualStructs must not be null.  Into it will be stored a bitfield
>     * representing which structs were compared to be non-equal.
>     */

And maybe add a note on the new argument here?

::: layout/style/nsStyleContext.cpp:260
(Diff revision 3)
> +  // whether to return a struct or not, since this->mBits might not yet
> +  // be correct (due to not calling ResolveSameStructsAs on it yet).
> +#define PEEK(struct_)                                                         \
> +  (IsGecko()                                                                  \
> +   ? PeekStyle##struct_()                                                     \
> +   : (aRelevantStructs & NS_STYLE_INHERIT_BIT(struct_))                       \

Ugh, I always need to remember the rules of precedence for this... Maybe wrap in parens the rightmost ternary expression as a whole so it's a bit clearer?
Attachment #8886922 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8886923 [details]
Bug 1380133 - Part 6: Remove FakeStyleContext.

https://reviewboard.mozilla.org/r/157670/#review164446

Lovely, one less hack :)
Comment on attachment 8886923 [details]
Bug 1380133 - Part 6: Remove FakeStyleContext.

https://reviewboard.mozilla.org/r/157670/#review164448
Attachment #8886923 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8886924 [details]
Bug 1380133 - Part 7: De-templatize CalcStyleDifference(Internal).

https://reviewboard.mozilla.org/r/157672/#review164450

Well, i guess this was the alternative to template + static for Gecko vs servo StyleContexts.

Fine anyway. r=me :)
Attachment #8886924 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8886928 [details]
Bug 1380133 - Part 8: Update test expectations.

https://reviewboard.mozilla.org/r/157680/#review164452
Attachment #8886928 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8886918 [details]
Bug 1380133 - Part 2: Take a ServoStyleContext* in Gecko_CalcStyleDifference.

https://reviewboard.mozilla.org/r/157660/#review164432

> Probably the left one should also be const, if while you're at it you could make the changes, it'd be awesome :)

There are many methods on nsStyleContext that don't modify the object but which aren't marked as const.  So that might be a bigger change than I'd like to address here.
Comment on attachment 8886920 [details]
Bug 1380133 - Part 4: Make CalcStyleDifferenceInternal not cache any new structs on ServoStyleContexts when in a traversal.

https://reviewboard.mozilla.org/r/157664/#review164440

> Probably this should include something about "during the parallel traversal", or something like that.

Ah, yeah, that commit message was from when an earlier patch always avoided caching on ServoStyleContexts, even when not in a traversal.  Will fix.
Comment on attachment 8886920 [details]
Bug 1380133 - Part 4: Make CalcStyleDifferenceInternal not cache any new structs on ServoStyleContexts when in a traversal.

https://reviewboard.mozilla.org/r/157664/#review164438

> FWIW I find these ThreadsafeFoo's methods behaviour a little bit too magic...

In earlier revisions of the patch I considered having a new |StyleFooForCalcDifference| or something, that would never cache for ServoStyleContexts, even when not in a traversal.  I didn't go with that in the end because it just forced other CalcStyleDifference callers (such as in the anonymous box restyling handlers) to explicitly call ResolveSameStructsAs, which felt a bit strange.

Happy to rename ThreadsafeStyleFoo to something else if you have a suggestion.
Comment on attachment 8886921 [details]
style: Pass old ComputedValues and old cached struct bits to Gecko_CalcStyleDifference.

https://reviewboard.mozilla.org/r/157666/#review164442

> I think now we can convert this to `&ComputedValues`, mind filing a followup/servo E-easy issue once this lands?

Filed https://github.com/servo/servo/issues/17795.
Comment on attachment 8886922 [details]
Bug 1380133 - Part 5: Call CalcStyleDifference with ServoStyleContexts instead of a FakeStyleContext wrapping a ServoComputedValues.

https://reviewboard.mozilla.org/r/157668/#review164444

> nit: Do you really need/want the static_cast?

It isn't necessary, we can silently truncate to uint32_t instead.

> nit: const

Oh, that function became unused.  I'll remove it.
Comment on attachment 8886929 [details]
Bug 1380133 - Part 9: Automated test for inheriting the start value of a transition.

https://reviewboard.mozilla.org/r/157682/#review164484
Attachment #8886929 - Flags: review?(cam) → review+
Attachment #8886917 - Attachment is obsolete: true
Attachment #8886921 - Attachment is obsolete: true
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/454d7232178d
Part 1: Minor reformatting and encapsulation. r=emilio
https://hg.mozilla.org/integration/autoland/rev/6f89b4ceda12
Part 2: Take a ServoStyleContext* in Gecko_CalcStyleDifference. r=emilio
https://hg.mozilla.org/integration/autoland/rev/22dabf04e560
Part 3: Remove nsPresContext argument to ServoStyleContext::ResolveSameStructsAs. r=emilio
https://hg.mozilla.org/integration/autoland/rev/cc720d72d024
Part 4: Make CalcStyleDifferenceInternal not cache any new structs on ServoStyleContexts when in a traversal. r=emilio
https://hg.mozilla.org/integration/autoland/rev/9a84b6988af9
Part 5: Call CalcStyleDifference with ServoStyleContexts instead of a FakeStyleContext wrapping a ServoComputedValues. r=emilio
https://hg.mozilla.org/integration/autoland/rev/cf561cad85f1
Part 6: Remove FakeStyleContext. r=emilio
https://hg.mozilla.org/integration/autoland/rev/1f1175528301
Part 7: De-templatize CalcStyleDifference(Internal). r=emilio
https://hg.mozilla.org/integration/autoland/rev/00edc2c32610
Part 8: Update test expectations. r=emilio
https://hg.mozilla.org/integration/autoland/rev/f835a60a13a3
Part 9: Automated test for inheriting the start value of a transition. r=heycam
Backed out for bustage at mozilla/KeyframeUtils.h: no 'object' file generated:

Servo part:
https://hg.mozilla.org/integration/autoland/rev/4df8bfcb4e9f1c64e99e04cb0d54376dae98b2de

Gecko part:
https://hg.mozilla.org/integration/autoland/rev/79ce273e2150149fade852d483d34691e479e465
https://hg.mozilla.org/integration/autoland/rev/f2298180d46791f41f6073f3a51480f172e903b3
https://hg.mozilla.org/integration/autoland/rev/1470d246276f52545d124105ffc16d21ff144981
https://hg.mozilla.org/integration/autoland/rev/8e48ee4ce7d4e8ab0a57668ea334fadd5dff31a6
https://hg.mozilla.org/integration/autoland/rev/0a99d8945fd9876a90e1619a33ae3637cb8817e4
https://hg.mozilla.org/integration/autoland/rev/d986a76643602972c0024a93f07a88b7fa685ce8
https://hg.mozilla.org/integration/autoland/rev/68de2711a4274b4ff35e56cb29a6dce6a173598f
https://hg.mozilla.org/integration/autoland/rev/b7486c8fc6f6fda4abcd859f3447e03a894ae55e
https://hg.mozilla.org/integration/autoland/rev/1248f5c75f47c583eec62a7e6768996c72a59477


Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f835a60a13a3f25824f872e25b9f8386fa218a17&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Bustage log: https://treeherder.mozilla.org/logviewer.html#?job_id=115932243&repo=autoland

05:18:54     INFO -  c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\obj-firefox\dist\include\mozilla/KeyframeUtils.h(19): error C2220: warning treated as error - no 'object' file generated
05:18:54     INFO -  c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\obj-firefox\dist\include\mozilla/KeyframeUtils.h(19): warning C4099: 'ServoComputedValues': type name first seen using 'class' now seen using 'struct'
05:18:54     INFO -  c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\layout\style\nsAnimationManager.h(20): note: see declaration of 'ServoComputedValues'
05:18:54     INFO -  c:/builds/moz2_slave/autoland-w32-00000000000000000/build/src/config/rules.mk:1050: recipe for target 'Unified_cpp_dom_animation0.obj' failed
05:18:54     INFO -  mozmake.EXE[5]: *** [Unified_cpp_dom_animation0.obj] Error 2
05:18:54     INFO -  mozmake.EXE[5]: Leaving directory 'c:/builds/moz2_slave/autoland-w32-00000000000000000/build/src/obj-firefox/dom/animation'
05:18:54     INFO -  c:/builds/moz2_slave/autoland-w32-00000000000000000/build/src/config/recurse.mk:73: recipe for target 'dom/animation/target' failed
05:18:54     INFO -  mozmake.EXE[4]: *** [dom/animation/target] Error 2
Flags: needinfo?(cam)
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #82)
> This also added a new heap write hazard:
> https://treeherder.mozilla.org/logviewer.html#?job_id=115931891&repo=autoland

Ah, thanks for pointing that out.  I realise my try syntax did a static analysis build, not a rooting analysis build, so I missed that. :(
Flags: needinfo?(cam)
Comment on attachment 8888624 [details]
Bug 1380133 - Part 10: Whitelist nsStyleContext::PeekStyleXXX for heap writes.

https://reviewboard.mozilla.org/r/159634/#review164988
Attachment #8888624 - Flags: review?(cam) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f779a8d870bb
Part 1: Minor reformatting and encapsulation. r=emilio
https://hg.mozilla.org/integration/autoland/rev/a69729c8be0d
Part 2: Take a ServoStyleContext* in Gecko_CalcStyleDifference. r=emilio
https://hg.mozilla.org/integration/autoland/rev/3683aef665bc
Part 3: Remove nsPresContext argument to ServoStyleContext::ResolveSameStructsAs. r=emilio
https://hg.mozilla.org/integration/autoland/rev/09aa8979e7b2
Part 4: Make CalcStyleDifferenceInternal not cache any new structs on ServoStyleContexts when in a traversal. r=emilio
https://hg.mozilla.org/integration/autoland/rev/f499740afbed
Part 5: Call CalcStyleDifference with ServoStyleContexts instead of a FakeStyleContext wrapping a ServoComputedValues. r=emilio
https://hg.mozilla.org/integration/autoland/rev/f9b6904e3e10
Part 6: Remove FakeStyleContext. r=emilio
https://hg.mozilla.org/integration/autoland/rev/c72f3f8c9c60
Part 7: De-templatize CalcStyleDifference(Internal). r=emilio
https://hg.mozilla.org/integration/autoland/rev/f2fce7aee9f5
Part 8: Update test expectations. r=emilio
https://hg.mozilla.org/integration/autoland/rev/6d725f6b7456
Part 9: Automated test for inheriting the start value of a transition. r=heycam
https://hg.mozilla.org/integration/autoland/rev/efc8385a9519
Part 10: Whitelist nsStyleContext::PeekStyleXXX for heap writes. r=heycam
Blocks: 1382742
Depends on: 1384719
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: