Closed
Bug 1330874
Opened 8 years ago
Closed 8 years ago
stylo: background fields are not correctly updated when it already has more than one values
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla53
| Tracking | Status | |
|---|---|---|
| firefox53 | --- | fixed |
People
(Reporter: xidorn, Assigned: emilio)
References
Details
Attachments
(1 file)
This is a pattern found from test_background_blend_mode.html.
Given the following code:
<!DOCTYPE html>
<div id="p"></div>
<script>
p.setAttribute("style", "background-attachment: fixed");
alert(getComputedStyle(p).backgroundAttachment);
p.setAttribute("style", "background-attachment: scroll, scroll");
alert(getComputedStyle(p).backgroundAttachment);
p.setAttribute("style", "background-attachment: scroll, fixed, scroll");
alert(getComputedStyle(p).backgroundAttachment);
</script>
The value of background-attachment changes between the first setting and the second setting. But the value isn't further updated in the third setting.
If we set the value back to a single "fixed" or "scroll", and run the third setting again, the value is properly updated.
I'm not sure whether it is an issue in Gecko side or Servo side. My guess is Servo side, since the binding function it calls just invokes a function Gecko is currently using.
|
||
Comment 1•8 years ago
|
||
This works in pure servo, so it's a bug in the property glue.
```
$ ./mach run -d scratch.html
ALERT: fixed
ALERT: scroll, scroll
ALERT: scroll, fixed, scroll
```
This is either a bug in fill_arrays, or a bug in how we clear the array.
|
|
||
Comment 2•8 years ago
|
||
Doing a bit of investigation it seems like Servo is writing the correct values to the image layer list (in particular, mAttachmentCount is 3 after the third setAttribute), however in nsComputedDOMStyle::DoGetBackgroundAttachment it is still two. I suspect the image layer array got copied at some point where it shouldn't have?
|
|
||
Comment 3•8 years ago
|
||
It seems like the length of the attachment array doesn't affect this, but the order does. Basically, every *other* setAttribute, we'll have this bug.
|
|
||
Comment 4•8 years ago
|
||
If I make it set background-color as well it works again.
wat.
|
|
||
Comment 5•8 years ago
|
||
But only if I set it to different values.
Is this a COW gone wrong?
|
|
||
Comment 6•8 years ago
|
||
So, in each iteration, Servo does a COW via .mutate_background() on the default struct. This works fine; Servo writes to it and all and you have the right set of values in the struct.
In every even iteration (and the first), this struct gets set to the previous one in finish_styling, dropping the existing value. This value is viewed by CSSOM fine.
In every odd iteration, this struct (and its associated ComputedValues) floats off into the ether somewhere, and finish_styling doesn't drop the existing value since it has an extra refcount (where? I don't know yet). This value sticks around till the next iteration and gets used by the CSSOM call.
In the getComputedValues after every even iteration, the value from the last iteration gets dropped as well. So you have two drops (of the Background struct) every even iteration, and no drops in the odd iterations. The anomalous drop -- the drop of the unread "ether" value from the previous iteration, seems to be hanging off the style context somewhere (backtrace of drop call below). Feels like a restyle issue?
frame #10: 0x000000010a08dbf9 XUL`style::gecko_bindings::sugar::ownership::HasArcFFI::release<style::gecko_properties::ComputedValues>(ptr=0x000000013a326640) + 185 at ownership.rs:105
frame #11: 0x000000010a177cdd XUL`geckoservo::glue::Servo_ComputedValues_Release(ptr=0x000000013a326640) + 29 at glue.rs:669
frame #12: 0x00000001066cada5 XUL`mozilla::RefPtrTraits<ServoComputedValues>::Release(aPtr=0x000000013a326640) + 21 at ServoArcTypeList.h:11
frame #13: 0x00000001066cad85 XUL`RefPtr<ServoComputedValues>::ConstRemovingRefPtrTraits<ServoComputedValues>::Release(aPtr=0x000000013a326640) + 21 at RefPtr.h:394
frame #14: 0x00000001066cad6a XUL`RefPtr<ServoComputedValues>::~RefPtr(this=0x00007fff5fbf6090) + 42 at RefPtr.h:78
frame #15: 0x00000001066b6505 XUL`RefPtr<ServoComputedValues>::~RefPtr(this=0x00007fff5fbf6090) + 21 at RefPtr.h:76
frame #16: 0x000000010681ee97 XUL`mozilla::OwningStyleContextSource::~OwningStyleContextSource(this=0x0000000139cc4420) + 295 at StyleContextSource.h:131
frame #17: 0x00000001067f0965 XUL`mozilla::OwningStyleContextSource::~OwningStyleContextSource(this=0x0000000139cc4420) + 21 at StyleContextSource.h:121
frame #18: 0x00000001067f2020 XUL`nsStyleContext::~nsStyleContext(this=0x0000000139cc43e8) + 608 at nsStyleContext.cpp:224
frame #19: 0x00000001067f4e45 XUL`nsStyleContext::~nsStyleContext(this=0x0000000139cc43e8) + 21 at nsStyleContext.cpp:182
frame #20: 0x000000010680138e XUL`nsStyleContext::Destroy(this=0x0000000139cc43e8) + 46 at nsStyleContext.cpp:1346
frame #21: 0x0000000103c4ba9d XUL`nsStyleContext::Release(this=0x0000000139cc43e8) + 157 at nsStyleContext.h:130
frame #22: 0x0000000103c4b9f5 XUL`mozilla::RefPtrTraits<nsStyleContext>::Release(aPtr=0x0000000139cc43e8) + 21 at RefPtr.h:40
frame #23: 0x0000000103c4b9d5 XUL`RefPtr<nsStyleContext>::ConstRemovingRefPtrTraits<nsStyleContext>::Release(aPtr=0x0000000139cc43e8) + 21 at RefPtr.h:394
frame #24: 0x0000000103c4b9ba XUL`RefPtr<nsStyleContext>::~RefPtr(this=0x00007fff5fbf63e0) + 42 at RefPtr.h:78
frame #25: 0x0000000103c2caa5 XUL`RefPtr<nsStyleContext>::~RefPtr(this=0x00007fff5fbf63e0) + 21 at RefPtr.h:76
frame #26: 0x00000001068e9e9d XUL`mozilla::ServoRestyleManager::RecreateStyleContexts(this=0x0000000139c85fc0, aElement=0x000000013a764d70, aParentContext=0x0000000139cc3ef0, aStyleSet=0x0000000139ca40f0, aChangeListToProcess=0x00007fff5fbf69b8) + 1357 at ServoRestyleManager.cpp:223
frame #27: 0x00000001068e9fd7 XUL`mozilla::ServoRestyleManager::RecreateStyleContexts(this=0x0000000139c85fc0, aElement=0x0000000139ce7c00, aParentContext=0x0000000139cc3c08, aStyleSet=0x0000000139ca40f0, aChangeListToProcess=0x00007fff5fbf69b8) + 1671 at ServoRestyleManager.cpp:234
frame #28: 0x00000001068e9fd7 XUL`mozilla::ServoRestyleManager::RecreateStyleContexts(this=0x0000000139c85fc0, aElement=0x0000000139c9a9c0, aParentContext=0x0000000000000000, aStyleSet=0x0000000139ca40f0, aChangeListToProcess=0x00007fff5fbf69b8) + 1671 at ServoRestyleManager.cpp:234
frame #29: 0x00000001068ea511 XUL`mozilla::ServoRestyleManager::ProcessPendingRestyles(this=0x0000000139c85fc0) + 561 at ServoRestyleManager.cpp:335
frame #30: 0x00000001068a957d XUL`mozilla::RestyleManagerHandle::Ptr::ProcessPendingRestyles(this=0x00007fff5fbf6b68) + 77 at RestyleManagerHandleInlines.h:75
frame #31: 0x00000001068b441c XUL`mozilla::PresShell::FlushPendingNotifications(this=0x0000000139c75c00, aFlush=(mFlushType = Style, mFlushAnimations = true)) + 1404 at PresShell.cpp:4096
frame #32: 0x00000001068b3de8 XUL`mozilla::PresShell::FlushPendingNotifications(this=0x0000000139c75c00, aType=Style) + 88 at PresShell.cpp:3986
frame #33: 0x0000000103f231d2 XUL`nsDocument::FlushPendingNotifications(this=0x0000000139c90000, aType=Style) + 1154 at nsDocument.cpp:7812
frame #34: 0x000000010672ebc3 XUL`nsComputedDOMStyle::UpdateCurrentStyleSources(this=0x000000012e0fd670, aNeedsLayoutFlush=false) + 259 at nsComputedDOMStyle.cpp:667
frame #35: 0x000000010672f65d XUL`nsComputedDOMStyle::GetPropertyCSSValue(this=0x000000012e0fd670, aPropertyName=0x00007fff5fbf7118, aRv=0x00007fff5fbf70b0) + 381 at nsComputedDOMStyle.cpp:830
frame #36: 0x000000010672e85a XUL`nsComputedDOMStyle::GetPropertyValue(this=0x000000012e0fd670, aPropertyName=0x00007fff5fbf7118, aReturn=0x00007fff5fbf72a8) + 74 at nsComputedDOMStyle.cpp:402
|
|
||
Comment 7•8 years ago
|
||
Btw, this only happens when a getComputedStyle() call exists (perhaps because it forces restyling?).
Still not sure how setting background-color changes this at all.
|
Assignee | |
Comment 8•8 years ago
|
||
I know why this happens, we're styling the element properly, but we're not reconstructing the style context since there's no change hint generated, I'll post a patch when I have decent internet.
|
|
Assignee | |
Comment 9•8 years ago
|
||
Hmmm... We should generate a neutral change though. I believe we're incorrectly skipping the diffing of style structs, not totally sure.
|
|
||
Comment 10•8 years ago
|
||
Yeah, that we're doing the diffing wrong was my initial guess (because of the effect of background-color), but it doesn't seem like that's the case. I'll have another look to confirm.
|
|
Assignee | |
Updated•8 years ago
|
Assignee: nobody → emilio+bugs
Status: NEW → ASSIGNED
|
|
Assignee | |
Comment 11•8 years ago
|
||
Sorry, should've commented earlier, we strip the neutral change, it's not a diffing problem. Taking this since I already have the patch half-backed.
| Comment hidden (mozreview-request) |
|
||
Comment 13•8 years ago
|
||
| mozreview-review | ||
Comment on attachment 8826839 [details]
Bug 1330874: Preserve neutral style changes for stylo.
https://reviewboard.mozilla.org/r/104714/#review105454
::: layout/style/nsStyleContext.h:402
(Diff revision 1)
> nsChangeHint aParentHintsNotHandledForDescendants,
> uint32_t* aEqualStructs,
> uint32_t* aSamePointerStructs);
>
> private:
> - template<class StyleContextLike>
> + template<class StyleContextLike, bool aStripNeutralChange = true>
Can you make this an enum, like
enum class NeutralChangeHandling {
Retain,
Strip,
};
and get rid of its default value, so that it's clearer at call sites what's happening.
::: layout/style/nsStyleContext.cpp:1269
(Diff revision 1)
> + // Given that, we can't strip the neutral change hint here, since it may
> + // provoke errors like bug 1330874.
An alternative would be to check if |*aEqualStructs != NS_STYLE_INHERIT_MASK|, and if so, re-add NeutralHint. You could do that here, or up in Gecko_CalcStyleDifference. That or your approach are both fine.
::: layout/style/nsStyleContext.cpp:1272
(Diff revision 1)
> + // NOTE(emilio): If we kept the rule node around in the style context too, and
> + // not just the ServoComputedValues, we could pointer-compare the rule nodes
> + // instead of handling NeutralChange separately. That being said, it doesn't
> + // seem like an immediately better solution.
I'm not sure checking the rule node is sufficient, since a change in an inherited property value could cause only a neutral change hint on this element.
Attachment #8826839 -
Flags: review?(cam) → review+
|
|
Assignee | |
Comment 14•8 years ago
|
||
| mozreview-review | ||
Comment on attachment 8826839 [details]
Bug 1330874: Preserve neutral style changes for stylo.
https://reviewboard.mozilla.org/r/104714/#review105464
::: layout/style/nsStyleContext.h:402
(Diff revision 1)
> nsChangeHint aParentHintsNotHandledForDescendants,
> uint32_t* aEqualStructs,
> uint32_t* aSamePointerStructs);
>
> private:
> - template<class StyleContextLike>
> + template<class StyleContextLike, bool aStripNeutralChange = true>
Sure
::: layout/style/nsStyleContext.cpp:1272
(Diff revision 1)
> + // NOTE(emilio): If we kept the rule node around in the style context too, and
> + // not just the ServoComputedValues, we could pointer-compare the rule nodes
> + // instead of handling NeutralChange separately. That being said, it doesn't
> + // seem like an immediately better solution.
You're absolutely right, I'll remove this comment before pushing.
|
||
Comment 15•8 years ago
|
||
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d34b196990a0
Preserve neutral style changes for stylo. r=heycam
|
||
Comment 16•8 years ago
|
||
| bugherder | ||
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•