1356916 - stylo: inline canvas width and height are not reflected

Status: RESOLVED FIXED

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

Description

[Hiroyuki Ikezoe (:hiro)]

layout/reftests/canvas/size-change-1.html fails because this.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

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

Comment 2

[Hiroyuki Ikezoe (:hiro)]

(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().

Comment 3

[Hiroyuki Ikezoe (:hiro)]

dom/canvas/test/test_bitmaprenderer.html also depends on this.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

(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 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1356916 - Call PostRestyleEvent() with the change hint obtained by Element::GetAttributeChangeHint in ServoRestyleManager::AttributeChanged.

Review commit: https://reviewboard.mozilla.org/r/137320/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/137320/

Comment 6

[Hiroyuki Ikezoe (:hiro)]

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 7

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

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 9

[Cameron McCormack (:heycam)]

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.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

(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!

Comment 11

[Hiroyuki Ikezoe (:hiro)]

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 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1356916 - Skip crash tests causing scrollbar-thumb atom leak.

Review commit: https://reviewboard.mozilla.org/r/138180/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138180/

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1356916 - Update reftest expectations.

Review commit: https://reviewboard.mozilla.org/r/138182/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138182/

Comment 14

[Hiroyuki Ikezoe (:hiro)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137320/diff/1-2/

Comment 15

[Cameron McCormack (:heycam)]

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

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8866575 [details]
Bug 1356916 - Skip crash tests causing scrollbar-thumb atom leak.

https://reviewboard.mozilla.org/r/138180/#review141472

Comment 17

[Cameron McCormack (:heycam)]

Comment on attachment 8866576 [details]
Bug 1356916 - Update reftest expectations.

https://reviewboard.mozilla.org/r/138182/#review141476

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Thank you for the review!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a137e3e4b7d
Additional reftest expectations fix. r=me

Comment 21

[Hiroyuki Ikezoe (:hiro)]

There were more passed tests that did not passed when I did a try on try server.

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f21116c45c78
https://hg.mozilla.org/mozilla-central/rev/afb494173e2d
https://hg.mozilla.org/mozilla-central/rev/aa71971b432b
https://hg.mozilla.org/mozilla-central/rev/3a137e3e4b7d