Closed Bug 1356916 Opened 3 years ago Closed 3 years ago

stylo: inline canvas width and height are not reflected

Categories

(Core :: CSS Parsing and Computation, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(3 files)

layout/reftests/canvas/size-change-1.html fails because this.
We need to call PostRestyleEvent in ServoRestyleManager::AttributeChanged [1], when we change canvas element's width and height.

[1] https://hg.mozilla.org/mozilla-central/file/17d8a1e278a9/layout/base/ServoRestyleManager.cpp#l504
(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> We need to call PostRestyleEvent in ServoRestyleManager::AttributeChanged
> [1], when we change canvas element's width and height.

This is wrong. We need to call FrameNeedsReflow (or something) somehow instead of PostRestyleEvent().
dom/canvas/test/test_bitmaprenderer.html also depends on this.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> > We need to call PostRestyleEvent in ServoRestyleManager::AttributeChanged
> > [1], when we change canvas element's width and height.
> 
> This is wrong. We need to call FrameNeedsReflow (or something) somehow
> instead of PostRestyleEvent().

I was probably correct.  In GeckoRestyleManager::AttributeChanged() we call PostRestyleEvent() with change hint obtained by Element::GetAttributeChangeHint.

Here is a try run with calling PostRestyleEvent() with the change hint.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=78bcfd813587cb0fe81f940aa90007b7a498f674

A dozen reftests passed.
Comment on attachment 8865690 [details]
Bug 1356916 - Call PostRestyleEvent() with the change hint obtained by Element::GetAttributeChangeHint in ServoRestyleManager::AttributeChanged.

https://reviewboard.mozilla.org/r/137320/#review140322

::: layout/base/ServoRestyleManager.cpp:568
(Diff revision 1)
> +  nsChangeHint hint = aElement->GetAttributeChangeHint(aAttribute, aModType);
> +  if (hint) {
> +    PostRestyleEvent(aElement, eRestyle_StyleAttribute, hint);
> +  }

This patch causes scrollbar-thumb atom leak, but if we do call FrameNeedsReflow() directly instead of PostRestyleEvent(), the leak does not happen. Also, if we do call ProcessRestyledFrames() instead of PostRestyleEvent(), the leak happens.
Comment on attachment 8865690 [details]
Bug 1356916 - Call PostRestyleEvent() with the change hint obtained by Element::GetAttributeChangeHint in ServoRestyleManager::AttributeChanged.

Oops. I did forgot to do hg qref -e to remove review request.
Attachment #8865690 - Flags: review?(cam)
A clue for the leak is StyleChildRangeForReconstruct() call [1] in nsCSSFrameConstructor::ContentRangeInserted(). Without this call no leak happens.

[1] https://hg.mozilla.org/mozilla-central/file/b21b974d60d3/layout/base/nsCSSFrameConstructor.cpp#l8019
Comment on attachment 8865690 [details]
Bug 1356916 - Call PostRestyleEvent() with the change hint obtained by Element::GetAttributeChangeHint in ServoRestyleManager::AttributeChanged.

https://reviewboard.mozilla.org/r/137320/#review140500

::: layout/base/ServoRestyleManager.cpp:570
(Diff revision 1)
>      primaryFrame->AttributeChanged(aNameSpaceID, aAttribute, aModType);
>    }
>  
> +  nsChangeHint hint = aElement->GetAttributeChangeHint(aAttribute, aModType);
> +  if (hint) {
> +    PostRestyleEvent(aElement, eRestyle_StyleAttribute, hint);

Why eRestyle_StyleAttribute?  I think it should be nsRestyleHint(0), because we don't want to update the style="" attribute rule (or do any other selector matching).  We just want to store the nsChangeHint to be picked up later to be processed.
(In reply to Cameron McCormack (:heycam) from comment #9)
> Comment on attachment 8865690 [details]
> Bug 1356916 - Call PostRestyleEvent() with the change hint obtained by
> Element::GetAttributeChangeHint in ServoRestyleManager::AttributeChanged.
> 
> https://reviewboard.mozilla.org/r/137320/#review140500
> 
> ::: layout/base/ServoRestyleManager.cpp:570
> (Diff revision 1)
> >      primaryFrame->AttributeChanged(aNameSpaceID, aAttribute, aModType);
> >    }
> >  
> > +  nsChangeHint hint = aElement->GetAttributeChangeHint(aAttribute, aModType);
> > +  if (hint) {
> > +    PostRestyleEvent(aElement, eRestyle_StyleAttribute, hint);
> 
> Why eRestyle_StyleAttribute?  I think it should be nsRestyleHint(0), because
> we don't want to update the style="" attribute rule (or do any other
> selector matching). 

Thank you Cameron! I did not think nsRestyleHint(0) is effective for, but yes, it worked.  

> We just want to store the nsChangeHint to be picked up
> later to be processed.

I did not know this fact. That's really what I wanted. Thank you!
I am almost giving up chasing the leak cause in this bug.  I've been running a leaked test (layout/forms/crashtests/166750-1.html) on gdb with watching ref count of URLExtraData for chrome://global/skin/scrollbars.css for two days, but not finished yet.  I should find out another way to catch it.
Comment on attachment 8865690 [details]
Bug 1356916 - Call PostRestyleEvent() with the change hint obtained by Element::GetAttributeChangeHint in ServoRestyleManager::AttributeChanged.

https://reviewboard.mozilla.org/r/137320/#review141468
Attachment #8865690 - Flags: review?(cam) → review+
Comment on attachment 8866575 [details]
Bug 1356916 - Skip crash tests causing scrollbar-thumb atom leak.

https://reviewboard.mozilla.org/r/138180/#review141472
Attachment #8866575 - Flags: review?(cam) → review+
Comment on attachment 8866576 [details]
Bug 1356916 - Update reftest expectations.

https://reviewboard.mozilla.org/r/138182/#review141476
Attachment #8866576 - Flags: review?(cam) → review+
Thank you for the review!
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f21116c45c78
Call PostRestyleEvent() with the change hint obtained by Element::GetAttributeChangeHint in ServoRestyleManager::AttributeChanged. r=heycam
https://hg.mozilla.org/integration/autoland/rev/afb494173e2d
Skip crash tests causing scrollbar-thumb atom leak. r=heycam
https://hg.mozilla.org/integration/autoland/rev/aa71971b432b
Update reftest expectations. r=heycam
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3a137e3e4b7d
Additional reftest expectations fix. r=me
There were more passed tests that did not passed when I did a try on try server.
Assignee: nobody → hikezoe
You need to log in before you can comment on or make changes to this bug.