Note: There are a few cases of duplicates in user autocompletion which are being worked on.

stylo: HackerRank menu hover animation flickers

ASSIGNED
Assigned to

Status

()

Core
CSS Parsing and Computation
P2
normal
ASSIGNED
10 days ago
11 hours ago

People

(Reporter: jdm, Assigned: heycam)

Tracking

(Blocks: 3 bugs)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(12 attachments, 5 obsolete attachments)

287 bytes, text/html
Details
2.21 KB, patch
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
emilio
: review+
Details | Review
59 bytes, text/x-review-board-request
emilio
: review+
Details | Review
59 bytes, text/x-review-board-request
emilio
: review+
Details | Review
59 bytes, text/x-review-board-request
emilio
: review+
Details | Review
59 bytes, text/x-review-board-request
emilio
: review+
Details | Review
59 bytes, text/x-review-board-request
emilio
: review+
Details | Review
59 bytes, text/x-review-board-request
emilio
: review+
Details | Review
59 bytes, text/x-review-board-request
emilio
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
(Reporter)

Description

10 days ago
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.
(Reporter)

Comment 1

10 days ago
Created attachment 8885439 [details]
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.
(Assignee)

Comment 6

8 days ago
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)
(Assignee)

Comment 7

8 days ago
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: → bug 1289622
Created attachment 8886506 [details] [diff] [review]
Mochitest

Here's a mochitest for you. Fails for me with Stylo enabled, passes with it disabled.
Comment hidden (mozreview-request)
(Assignee)

Comment 11

7 days ago
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.
Duplicate of this bug: 1378972
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.
(Assignee)

Comment 14

5 days ago
OK, hopefully I haven't duplicated too much work then...
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Blocks: 1364484
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 days ago
Attachment #8886925 - Attachment is obsolete: true
(Assignee)

Updated

2 days ago
Attachment #8886926 - Attachment is obsolete: true
(Assignee)

Updated

2 days ago
Attachment #8886927 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 51

2 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=175f4c9d6f78922afe59cffa7e4358cf7ce3f267&group_state=expanded
(Assignee)

Updated

2 days ago
Attachment #8886917 - Flags: review?(emilio+bugs)
Attachment #8886921 - Flags: review?(emilio+bugs)

Comment 52

2 days ago
mozreview-review
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 53

2 days ago
mozreview-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 54

2 days ago
mozreview-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 55

2 days ago
mozreview-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 56

2 days ago
mozreview-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 57

2 days ago
mozreview-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 58

2 days ago
mozreview-review
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 59

2 days ago
mozreview-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 60

2 days ago
mozreview-review
Comment on attachment 8886923 [details]
Bug 1380133 - Part 6: Remove FakeStyleContext.

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

Lovely, one less hack :)

Comment 61

2 days ago
mozreview-review
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 62

2 days ago
mozreview-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 63

2 days ago
mozreview-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+
(Assignee)

Comment 64

2 days ago
mozreview-review-reply
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.
(Assignee)

Comment 65

2 days ago
mozreview-review-reply
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.
(Assignee)

Comment 66

2 days ago
mozreview-review-reply
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.
(Assignee)

Comment 67

2 days ago
mozreview-review-reply
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.
(Assignee)

Comment 68

2 days ago
mozreview-review-reply
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.
(Assignee)

Comment 69

2 days ago
mozreview-review
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 days ago
Attachment #8886917 - Attachment is obsolete: true
(Assignee)

Updated

2 days ago
Attachment #8886921 - Attachment is obsolete: true
Duplicate of this bug: 1375390

Comment 80

a day ago
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)
This also added a new heap write hazard: https://treeherder.mozilla.org/logviewer.html#?job_id=115931891&repo=autoland

Updated

20 hours ago
Duplicate of this bug: 1382742
(Assignee)

Comment 84

17 hours ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 95

15 hours ago
mozreview-review
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Duplicate of this bug: 1381670
Duplicate of this bug: 1382742

Comment 101

14 hours ago
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: 1382956
You need to log in before you can comment on or make changes to this bug.