Bug 61986 – Verify cloning <meter> and <progress> work

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Dominic Cooney]

This is suspected of being broken by the same cause as bug 61909.

[Hajime Morrita]

Created attachment 95859 [details]
Patch

[Kent Tamura]

Comment on attachment 95859 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95859&action=review

> LayoutTests/ChangeLog:5
> +        REGRESSION: Cloned text <progress> and <meter> doesn't work well

* It would be helpful if you write a specific revision made the regression.
* "text <progress>"?
* I know this bug makes various problems.  However, we had better explain what problems are solved.

[Dominic Cooney]

Comment on attachment 95859 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95859&action=review

> Source/WebCore/html/shadow/ProgressShadowElement.h:81
> +    virtual PassRefPtr<Element> cloneElementWithoutAttributesAndChildren() const;

Isn’t this protected in Element? Why publicize it?

[Dominic Cooney]

Comment on attachment 95859 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95859&action=review

> LayoutTests/fast/dom/HTMLMeterElement/meter-clone.html:1
> +<!html>

This test will need to be skipped on win DRT because it does not have layoutTestController.shadowRoot yet IIRC.

> LayoutTests/fast/dom/HTMLMeterElement/meter-clone.html:16
> +targetShadowRoot = layoutTestController.shadowRoot(target);

Shouldn’t these be guarded by if (window.layoutTestController)?

You could make a test that works in the browser by inspecting the cloned element’s actual size. (And also query the shadow root when in DRT.)

[Kent Tamura]

Comment on attachment 95859 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95859&action=review

r- because of various feedbacks.

> LayoutTests/fast/dom/HTMLProgressElement/progress-clone.html:1
> +<!html>

!!!

[Kent Tamura]

I think a cloned <meter> can cause a crash because of m_value is initialized in createShadowSubtree().

[Kent Tamura]

(In reply to comment #6)
> I think a cloned <meter> can cause a crash because of m_value is initialized in createShadowSubtree().

<progress> too.

[Dominic Cooney]

*** Bug 61987 has been marked as a duplicate of this bug. ***

[Hajime Morrita]

Created attachment 95864 [details]
Patch

[Hajime Morrita]

Comment on attachment 95864 [details]
Patch

Found this isn't ready.

[Dominic Cooney]

I don’t think m_value will cause a crash. As long as createShadowSubtree is called, m_value will be initialized. But it may be removed from the tree when the shadow is clobbered. So it will cause wonky behavior but I don’t think it will cause a crash.

[Kent Tamura]

(In reply to comment #11)
> I don’t think m_value will cause a crash. As long as createShadowSubtree is called, m_value will be initialized. But it may be removed from the tree when the shadow is clobbered. So it will cause wonky behavior but I don’t think it will cause a crash.

Oh, you're right. m_value keeps to refere an incorrect node.
Fortunately m_value is RefPtr<>.  So it would not be a security issue.

[Hajime Morrita]

Created attachment 95881 [details]
Patch

[Kent Tamura]

Comment on attachment 95881 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95881&action=review

> LayoutTests/fast/dom/HTMLMeterElement/meter-clone.html:17
> +
> +targetShadowRoot = layoutTestController.shadowRoot(target);
> +clonedShadowRoot = layoutTestController.shadowRoot(cloned);

Please show something if there is no layoutTestController.

> LayoutTests/fast/dom/HTMLProgressElement/progress-clone.html:16
> +targetShadowRoot = layoutTestController.shadowRoot(target);

ditto.

> Source/WebCore/dom/ShadowRoot.cpp:136
> +Element* ShadowRoot::getElementByIdSearchingUnindexed(const AtomicString& id) const

Is it necessary for <progress> and <meter>?

[Hajime Morrita]

Created attachment 95882 [details]
Added missed fix on test htmls

[Hajime Morrita]

Kent-san, Dominic, thank you for taking a look!
I updated the patch.

(In reply to comment #2
> (From update of attachment 95859 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95859&action=review
> 
> > LayoutTests/ChangeLog:5
> > +        REGRESSION: Cloned text <progress> and <meter> doesn't work well
> 
> * It would be helpful if you write a specific revision made the regression.
I added suspicious revisions for this regression.

> * "text <progress>"?
Oops. Removed "text".

> * I know this bug makes various problems.  However, we had better explain what problems are solved.
Clarified the fix in detail.


(In reply to comment #3)
> (From update of attachment 95859 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95859&action=review
> 
> > Source/WebCore/html/shadow/ProgressShadowElement.h:81
> > +    virtual PassRefPtr<Element> cloneElementWithoutAttributesAndChildren() const;
> 
> Isn’t this protected in Element? Why publicize it?
Moved them to private.


(In reply to comment #5)
> (From update of attachment 95859 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95859&action=review
> 
> r- because of various feedbacks.
> 
> > LayoutTests/fast/dom/HTMLProgressElement/progress-clone.html:1
> > +<!html>
> 
> !!!
Oopps. Removed...

(In reply to comment #6)
> I think a cloned <meter> can cause a crash because of m_value is initialized in createShadowSubtree().
I removed m_value fields and replaced it with on-demand lookups.

[Hajime Morrita]

Ping?

(In reply to comment #16)
> Kent-san, Dominic, thank you for taking a look!
> I updated the patch.
> 
> (In reply to comment #2
> > (From update of attachment 95859 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=95859&action=review
> > 
> > > LayoutTests/ChangeLog:5
> > > +        REGRESSION: Cloned text <progress> and <meter> doesn't work well
> > 
> > * It would be helpful if you write a specific revision made the regression.
> I added suspicious revisions for this regression.
> 
> > * "text <progress>"?
> Oops. Removed "text".
> 
> > * I know this bug makes various problems.  However, we had better explain what problems are solved.
> Clarified the fix in detail.
> 
> 
> (In reply to comment #3)
> > (From update of attachment 95859 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=95859&action=review
> > 
> > > Source/WebCore/html/shadow/ProgressShadowElement.h:81
> > > +    virtual PassRefPtr<Element> cloneElementWithoutAttributesAndChildren() const;
> > 
> > Isn’t this protected in Element? Why publicize it?
> Moved them to private.
> 
> 
> (In reply to comment #5)
> > (From update of attachment 95859 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=95859&action=review
> > 
> > r- because of various feedbacks.
> > 
> > > LayoutTests/fast/dom/HTMLProgressElement/progress-clone.html:1
> > > +<!html>
> > 
> > !!!
> Oopps. Removed...
> 
> (In reply to comment #6)
> > I think a cloned <meter> can cause a crash because of m_value is initialized in createShadowSubtree().
> I removed m_value fields and replaced it with on-demand lookups.

[Kent Tamura]

(In reply to comment #17)
> Ping?

Did you read Comment #14 ?

[Hajime Morrita]

(In reply to comment #18)
> (In reply to comment #17)
> > Ping?
> 
> Did you read Comment #14 ?
Oops, no. I'll take care of them. Thank you for pointing that out.

[Kent Tamura]

Comment on attachment 95882 [details]
Added missed fix on test htmls

View in context: https://bugs.webkit.org/attachment.cgi?id=95882&action=review

> Source/WebCore/html/shadow/MeterShadowElement.h:61
> +    virtual PassRefPtr<Element> cloneElementWithoutAttributesAndChildren() const;

We don't need such change any more.

> Source/WebCore/html/shadow/MeterShadowElement.h:88
> +    virtual PassRefPtr<Element> cloneElementWithoutAttributesAndChildren() const;

ditto.

> Source/WebCore/html/shadow/ProgressShadowElement.h:62
> +    virtual PassRefPtr<Element> cloneElementWithoutAttributesAndChildren() const;

ditto.

> Source/WebCore/html/shadow/ProgressShadowElement.h:87
> +    virtual PassRefPtr<Element> cloneElementWithoutAttributesAndChildren() const;

ditto.

[Hajime Morrita]

Created attachment 96062 [details]
Patch

[Hajime Morrita]

So I updated the patch to address following issues...
(In reply to comment #14)
> (From update of attachment 95881 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95881&action=review
> 
> > LayoutTests/fast/dom/HTMLMeterElement/meter-clone.html:17
> > +
> > +targetShadowRoot = layoutTestController.shadowRoot(target);
> > +clonedShadowRoot = layoutTestController.shadowRoot(cloned);
> 
> Please show something if there is no layoutTestController.
Done.

> 
> > LayoutTests/fast/dom/HTMLProgressElement/progress-clone.html:16
> > +targetShadowRoot = layoutTestController.shadowRoot(target);
> 
> ditto.
Fixed too.

> 
> > Source/WebCore/dom/ShadowRoot.cpp:136
> > +Element* ShadowRoot::getElementByIdSearchingUnindexed(const AtomicString& id) const
> 
> Is it necessary for <progress> and <meter>?
Yes. And I guess we need this also for <details> and <summary>.

[Kent Tamura]

Comment on attachment 95881 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95881&action=review

>>> Source/WebCore/dom/ShadowRoot.cpp:136
>>> +Element* ShadowRoot::getElementByIdSearchingUnindexed(const AtomicString& id) const
>> 
>> Is it necessary for <progress> and <meter>?
> 
> Yes. And I guess we need this also for <details> and <summary>.

Could you explain why is is possible that an element is not indexed?

[Kent Tamura]

Comment on attachment 96062 [details]
Patch

r- because of Comment #20

[Hajime Morrita]

Hmm. I need to run these tests against Tot....

Anyway,
> Could you explain why is is possible that an element is not indexed?
That's because HTMLConsructionSite calls setAttributeMap() before the element is attached to the tree,
But setAttributeMap() triggers parseMappedAttribute(), which requires the shadow node lookup.

[Kent Tamura]

(In reply to comment #25)
> Hmm. I need to run these tests against Tot....
> 
> Anyway,
> > Could you explain why is is possible that an element is not indexed?
> That's because HTMLConsructionSite calls setAttributeMap() before the element is attached to the tree,
> But setAttributeMap() triggers parseMappedAttribute(), which requires the shadow node lookup.

Oh, I understand.  Element::insertedIntoDocument() for shadow nodes is not called until the host element is inserted into the document...

[Hajime Morrita]

Created attachment 96067 [details]
Changed to have test cases only.

[Kent Tamura]

Comment on attachment 96067 [details]
Changed to have test cases only.

Yeah, this is enough for now.

[Hajime Morrita]

Committed r88149: <http://trac.webkit.org/changeset/88149>