Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(159)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 948793003: Fix inconsistent frame detach behavior of ContainerNode::parserRemoveChild. (Closed)

Created:
5 years, 10 months ago by kouhei (in TOK)
Modified:
5 years, 10 months ago
CC:
blink-reviews-dom_chromium.org, dglazkov+blink, sof, eae+blinkwatch, rwlbuis, jschuh
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Fix inconsistent frame detach behavior of ContainerNode::parserRemoveChild. Before this CL, ContainerNode::parserRemoveChild didn't detach descendent <frame>s like ContainerNode::removeChild did. ContainerNode::parserRemoveChild is called from document parsers for reparenting, and this may leave the <frame>s in broken state. This CL fixes the issue by issuing ChildFrameDisconnector from parserRemoveChild. Node::updateAncestorConnectedSubframeCountForRemoval is removed, as the decrement is now issued from HTMLFrameOwnerElement::clearContentFrame called from fromChildFrameDisconnector. Rebaselined: fast/parser/adoption-agency-crash-01.html fast/parser/adoption-agency-crash-03.html - The iframes parser removed shouldn't be invoked onload handler anyway. BUG=456518 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190980

Patch Set 1 #

Total comments: 7

Patch Set 2 : rewrite tests #

Patch Set 3 : rebaseline #

Unified diffs Side-by-side diffs Delta from patch set Stats (+28 lines, -18 lines) Patch
M LayoutTests/fast/css/stylesheet-candidate-nodes-crash-expected.txt View 1 2 1 chunk +6 lines, -0 lines 0 comments Download
A LayoutTests/fast/frames/reparented-iframe-cleared-contentWindow.html View 1 1 chunk +17 lines, -0 lines 0 comments Download
A + LayoutTests/fast/frames/reparented-iframe-cleared-contentWindow-expected.txt View 1 1 chunk +1 line, -2 lines 0 comments Download
M LayoutTests/fast/parser/adoption-agency-crash-01-expected.txt View 1 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/parser/adoption-agency-crash-03-expected.txt View 1 1 chunk +1 line, -2 lines 0 comments Download
M Source/core/dom/ContainerNode.cpp View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M Source/core/dom/Node.h View 1 2 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/dom/Node.cpp View 1 2 1 chunk +0 lines, -11 lines 0 comments Download

Messages

Total messages: 13 (3 generated)
kouhei (in TOK)
Would you take a look?
5 years, 10 months ago (2015-02-23 06:27:12 UTC) #2
kouhei (in TOK)
+morrita: Would you take a look?
5 years, 10 months ago (2015-02-23 06:33:29 UTC) #4
hayato
Could you add me to bug 456518? I can't see it.
5 years, 10 months ago (2015-02-23 07:05:28 UTC) #5
Yuta Kitamura
If there's nobody else who can review this, I would rubber- stamp, but a more ...
5 years, 10 months ago (2015-02-23 07:54:41 UTC) #6
kouhei (in TOK)
Thanks! Rewrote tests. https://codereview.chromium.org/948793003/diff/1/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (left): https://codereview.chromium.org/948793003/diff/1/Source/core/dom/Node.cpp#oldcode2269 Source/core/dom/Node.cpp:2269: node->decrementConnectedSubframeCount(count); On 2015/02/23 07:54:41, Yuta Kitamura ...
5 years, 10 months ago (2015-02-23 11:51:30 UTC) #7
kouhei (in TOK)
Friendly ping?
5 years, 10 months ago (2015-02-26 03:45:57 UTC) #8
Yuta Kitamura
Okay, lgtm
5 years, 10 months ago (2015-02-27 03:32:40 UTC) #9
kouhei (in TOK)
Thanks!
5 years, 10 months ago (2015-02-27 04:04:07 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/948793003/40001
5 years, 10 months ago (2015-02-27 04:05:01 UTC) #12
commit-bot: I haz the power
5 years, 10 months ago (2015-02-27 07:11:14 UTC) #13
Message was sent while issue was closed. 
Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190980

Powered by Google App Engine
This is Rietveld 408576698