Bug 92899

Created attachment 156653 [details] WIP

I will explain this bug by using the attached test case example. The DOM tree of the test case looks like: #box ------------ sr1 | | | +--<content> | +---#item ------ sr2 | +---<style> | +---<div> To render the test case, ElementShadow::invalidateDistribution is invoked because of <content>. In ElementShadow::invalidateDistribution for sr1's <content>, #box->detach() and #box->lazyAttach(...) are invoked. So, all children of the host have childNeedsStyleRecalc, i.e. #item has childNeedsStyleRecalc true. As #box was detached, #box's Element::recalcStyle invokes reattach() and only clears #box's childNeedsStyleRecalc. When the class attribute of <div> in sr2 is changed to be "selected", Node::setNeedsStyleRecalc is invoked and Node::markAncestorsWithChildNeedsStyleRecalc is invoked. In Node::markAncestorsWithChildNeedsStyleRecalc, walk up from <div> and set childNeedsStyleRecalc until childNeedsStyleRecalc has been already set or no parent is found. i.e. <sr2> --> #item --> #box --> ... As #item's childNeedsStyleRecalc is not cleared, so document()'s childNeedsStyleRecalc is not updated. So, if (document()->childNeedsStyleRecalc()) document()->scheduleStyleRecalc(); Document::scheduleStyleRecalc is not invoked. No re-layout will be executed. Best regards, Takashi Sakamoto

Created attachment 159102 [details] Patch

Comment on attachment 159102 [details] Patch Attachment 159102 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13521449 New failing tests: editing/shadow/delete-characters-in-distributed-node-crash.html

Created attachment 159144 [details] Archive of layout-test-results from gce-cr-linux-08 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-08 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 159102 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159102&action=review I think Morrita-san should look over this. You're removing the logic that he's added :) > Source/WebCore/dom/Element.cpp:-1001 > - detachAsNode(); Is this the only callsite of this method? Perhaps we should remove the method too? > Source/WebCore/dom/ElementShadow.cpp:-209 > - host->lazyAttach(Node::DoNotSetAttached); Ditto with DoNotSetAttached.

Comment on attachment 159102 [details] Patch Attachment 159102 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13531078 New failing tests: editing/shadow/delete-characters-in-distributed-node-crash.html

Created attachment 159161 [details] Archive of layout-test-results from gce-cr-linux-07 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-07 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 159323 [details] Patch

Comment on attachment 159323 [details] Patch Looks sane. And code becomes more simpler.

Comment on attachment 159323 [details] Patch Rejecting attachment 159323 [details] from commit-queue. New failing tests: editing/shadow/delete-characters-in-distributed-node-crash.html Full output: http://queues.webkit.org/results/13543188

Created attachment 159334 [details] Archive of layout-test-results from gce-cq-04 The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: gce-cq-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 159323 [details] Patch Attachment 159323 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13529831 New failing tests: editing/shadow/delete-characters-in-distributed-node-crash.html

Created attachment 159337 [details] Archive of layout-test-results from gce-cr-linux-07 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-07 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 159323 [details] Patch Attachment 159323 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13533716 New failing tests: editing/shadow/delete-characters-in-distributed-node-crash.html

Created attachment 159346 [details] Archive of layout-test-results from gce-cr-linux-06 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-06 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 159102 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159102&action=review >> Source/WebCore/dom/Element.cpp:-1001 >> - detachAsNode(); > > Is this the only callsite of this method? Perhaps we should remove the method too? I see. I removed. >> Source/WebCore/dom/ElementShadow.cpp:-209 >> - host->lazyAttach(Node::DoNotSetAttached); > > Ditto with DoNotSetAttached. Done.

I looked at cq- log and found that the test, delete-characters-in-distributed-node-crash passes. However one more newline character is added to the actual text: --- /tmp/layout-test-results/editing/shadow/delete-characters-in-distributed-node-crash-expected.txt +++ /tmp/layout-test-results/editing/shadow/delete-characters-in-distributed-node-crash-actual.txt @@ -1,4 +1,3 @@ This tests the deletion of text in distributed node does not crash. To run it outside of DRT, you must delete text, 'foo', manually. - PASS I'm now thinking of how to fix this issue. Best regards, Takashi Sakamoto

Created attachment 159643 [details] Patch

Comment on attachment 159643 [details] Patch Attachment 159643 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13547571 New failing tests: editing/shadow/delete-characters-in-distributed-node-crash.html

Created attachment 159712 [details] Archive of layout-test-results from gce-cr-linux-05 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-05 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 159823 [details] Patch

(In reply to comment #22) > Created an attachment (id=159823) [details] > Patch Since I heard that cq was restarted, I uploaded the same patch (locally the failed test passes) again. However, I'm sorry. I forgot to update reviewer line. If I obtain cq- again, I will modify test expectation and will fix the ChangeLog. If not, I will fix the ChangeLog. Best regards, Takashi Sakamoto

Comment on attachment 159823 [details] Patch Attachment 159823 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13566093 New failing tests: editing/shadow/delete-characters-in-distributed-node-crash.html

Created attachment 159847 [details] Archive of layout-test-results from gce-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 159853 [details] Patch

Comment on attachment 159853 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159853&action=review > LayoutTests/platform/chromium/TestExpectations:3515 > +BUGWK92899 LINUX : editing/shadow/delete-characters-in-distributed-node-crash.html = PASS TEXT I think it would be better to make the test more robust instead of writing test expectations here.

Created attachment 159856 [details] Patch

(In reply to comment #27) > (From update of attachment 159853 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=159853&action=review > > > LayoutTests/platform/chromium/TestExpectations:3515 > > +BUGWK92899 LINUX : editing/shadow/delete-characters-in-distributed-node-crash.html = PASS TEXT > > I think it would be better to make the test more robust instead of writing test expectations here. Thank you for advice. Done. Best regards, Takashi Sakamoto

Comment on attachment 159856 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159856&action=review > LayoutTests/editing/shadow/delete-characters-in-distributed-node-crash.html:16 > +document.getElementById("wrapper").innerHTML = "PASS"; It seems more robust than before. Good.

Comment on attachment 159856 [details] Patch let's see how the bot takes this.

Comment on attachment 159856 [details] Patch Rejecting attachment 159856 [details] from commit-queue. New failing tests: editing/shadow/delete-characters-in-distributed-node-crash.html Full output: http://queues.webkit.org/results/13557198

Created attachment 159867 [details] Archive of layout-test-results from gce-cq-02 The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: gce-cq-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 159856 [details] Patch Attachment 159856 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13561172 New failing tests: editing/shadow/delete-characters-in-distributed-node-crash.html

Created attachment 159868 [details] Archive of layout-test-results from gce-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 159856 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159856&action=review >> LayoutTests/editing/shadow/delete-characters-in-distributed-node-crash.html:16 >> +document.getElementById("wrapper").innerHTML = "PASS"; > > It seems more robust than before. Good. Oh, you have to update the expected result, too.

Created attachment 159873 [details] Patch for landing

(In reply to comment #36) > (From update of attachment 159856 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=159856&action=review > > >> LayoutTests/editing/shadow/delete-characters-in-distributed-node-crash.html:16 > >> +document.getElementById("wrapper").innerHTML = "PASS"; > > > > It seems more robust than before. Good. > > Oh, you have to update the expected result, too. Done. Best regards, Takashi Sakamoto

Comment on attachment 159873 [details] Patch for landing Clearing flags on attachment: 159873 Committed r126275: <http://trac.webkit.org/changeset/126275>

All reviewed patches have been landed. Closing bug.

Strangely, the test added in this patch (fast/dom/shadow/shadowdom-dynamic-styling.html) fails on EFL port (we have SHADOW_DOM enabled): -PASS window.getComputedStyle(target).backgroundColor is "rgb(255, 0, 0)" +FAIL window.getComputedStyle(target).backgroundColor should be rgb(255, 0, 0). Was rgba(0, 0, 0, 0). Bug 94690 was filed. If you have any idea of what the reason could be, help would be much appreciated.