Bug 61986

Summary:

Verify cloning <meter> and <progress> work

Product:

WebKit

Reporter:

Dominic Cooney <dominicc>

Component:

DOM

Assignee:

Hajime Morrita <morrita>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

morrita, tkent

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

61983

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Added missed fix on test htmls	none
Patch	none
Changed to have test cases only.	tkent: review+

Dominic Cooney

Reported 2011-06-02 19:57:28 PDT

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

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
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Hajime Morrita

Comment 1 2011-06-02 23:46:39 PDT

Created attachment 95859 [details] Patch

Kent Tamura

Comment 2 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.

Dominic Cooney

Comment 3 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?

Dominic Cooney

Comment 4 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.)

Kent Tamura

Comment 5 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> !!!

Kent Tamura

Comment 6 2011-06-03 00:33:23 PDT

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

Kent Tamura

Comment 7 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.

Dominic Cooney

Comment 8 2011-06-03 00:34:24 PDT

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

Hajime Morrita

Comment 9 2011-06-03 00:44:42 PDT

Created attachment 95864 [details] Patch

Hajime Morrita

Comment 10 2011-06-03 00:47:57 PDT

Comment on attachment 95864 [details] Patch Found this isn't ready.

Dominic Cooney

Comment 11 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.

Kent Tamura

Comment 12 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.

Hajime Morrita

Comment 13 2011-06-03 03:06:13 PDT

Created attachment 95881 [details] Patch

Kent Tamura

Comment 14 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>?

Hajime Morrita

Comment 15 2011-06-03 03:16:13 PDT

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

Hajime Morrita

Comment 16 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.

Hajime Morrita

Comment 17 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.

Kent Tamura

Comment 18 2011-06-05 20:49:21 PDT

(In reply to comment #17) > Ping? Did you read Comment #14 ?

Hajime Morrita

Comment 19 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.

Kent Tamura

Comment 20 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.

Hajime Morrita

Comment 21 2011-06-05 21:13:38 PDT

Created attachment 96062 [details] Patch

Hajime Morrita

Comment 22 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>.

Kent Tamura

Comment 23 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?

Kent Tamura

Comment 24 2011-06-05 21:23:44 PDT

Comment on attachment 96062 [details] Patch r- because of Comment #20

Hajime Morrita

Comment 25 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.

Kent Tamura

Comment 26 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...

Hajime Morrita

Comment 27 2011-06-06 00:25:02 PDT

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

Kent Tamura

Comment 28 2011-06-06 02:02:53 PDT

Comment on attachment 96067 [details] Changed to have test cases only. Yeah, this is enough for now.

Hajime Morrita

Comment 29 2011-06-06 02:30:10 PDT

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

Note You need to log in before you can comment on or make changes to this bug.