Bug 61986 - Verify cloning <meter> and <progress> work
Summary: Verify cloning <meter> and <progress> work
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
: 61987 (view as bug list)
Depends on:
Blocks: 61983
  Show dependency treegraph
 
Reported: 2011-06-02 19:57 PDT by Dominic Cooney
Modified: 2011-06-06 02:30 PDT (History)
2 users (show)

See Also:


Attachments
Patch (9.37 KB, patch)
2011-06-02 23:46 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (9.93 KB, patch)
2011-06-03 00:44 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (16.45 KB, patch)
2011-06-03 03:06 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Added missed fix on test htmls (16.43 KB, patch)
2011-06-03 03:16 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (16.85 KB, patch)
2011-06-05 21:13 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Changed to have test cases only. (6.01 KB, patch)
2011-06-06 00:25 PDT, Hajime Morrita
tkent: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominic Cooney 2011-06-02 19:57:28 PDT
This is suspected of being broken by the same cause as bug 61909.
Comment 1 Hajime Morrita 2011-06-02 23:46:39 PDT
Created attachment 95859 [details]
Patch
Comment 2 Kent Tamura 2011-06-02 23:55:34 PDT
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.
Comment 3 Dominic Cooney 2011-06-03 00:05:23 PDT
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?
Comment 4 Dominic Cooney 2011-06-03 00:23:11 PDT
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.)
Comment 5 Kent Tamura 2011-06-03 00:25:31 PDT
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>

!!!
Comment 6 Kent Tamura 2011-06-03 00:33:23 PDT
I think a cloned <meter> can cause a crash because of m_value is initialized in createShadowSubtree().
Comment 7 Kent Tamura 2011-06-03 00:33:47 PDT
(In reply to comment #6)
> I think a cloned <meter> can cause a crash because of m_value is initialized in createShadowSubtree().

<progress> too.
Comment 8 Dominic Cooney 2011-06-03 00:34:24 PDT
*** Bug 61987 has been marked as a duplicate of this bug. ***
Comment 9 Hajime Morrita 2011-06-03 00:44:42 PDT
Created attachment 95864 [details]
Patch
Comment 10 Hajime Morrita 2011-06-03 00:47:57 PDT
Comment on attachment 95864 [details]
Patch

Found this isn't ready.
Comment 11 Dominic Cooney 2011-06-03 01:01:39 PDT
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.
Comment 12 Kent Tamura 2011-06-03 01:08:25 PDT
(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.
Comment 13 Hajime Morrita 2011-06-03 03:06:13 PDT
Created attachment 95881 [details]
Patch
Comment 14 Kent Tamura 2011-06-03 03:14:50 PDT
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>?
Comment 15 Hajime Morrita 2011-06-03 03:16:13 PDT
Created attachment 95882 [details]
Added missed fix on test htmls
Comment 16 Hajime Morrita 2011-06-03 03:17:44 PDT
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.
Comment 17 Hajime Morrita 2011-06-05 20:43:13 PDT
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.
Comment 18 Kent Tamura 2011-06-05 20:49:21 PDT
(In reply to comment #17)
> Ping?

Did you read Comment #14 ?
Comment 19 Hajime Morrita 2011-06-05 20:55:56 PDT
(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.
Comment 20 Kent Tamura 2011-06-05 20:57:56 PDT
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.
Comment 21 Hajime Morrita 2011-06-05 21:13:38 PDT
Created attachment 96062 [details]
Patch
Comment 22 Hajime Morrita 2011-06-05 21:18:50 PDT
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>.
Comment 23 Kent Tamura 2011-06-05 21:23:13 PDT
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?
Comment 24 Kent Tamura 2011-06-05 21:23:44 PDT
Comment on attachment 96062 [details]
Patch

r- because of Comment #20
Comment 25 Hajime Morrita 2011-06-05 21:36:36 PDT
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.
Comment 26 Kent Tamura 2011-06-05 22:01:09 PDT
(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...
Comment 27 Hajime Morrita 2011-06-06 00:25:02 PDT
Created attachment 96067 [details]
Changed to have test cases only.
Comment 28 Kent Tamura 2011-06-06 02:02:53 PDT
Comment on attachment 96067 [details]
Changed to have test cases only.

Yeah, this is enough for now.
Comment 29 Hajime Morrita 2011-06-06 02:30:10 PDT
Committed r88149: <http://trac.webkit.org/changeset/88149>