The ASSERT in appendNode() should not fire when OBJECTS are cloned.

The ASSERT in appendNode() should not fire when OBJECTS are cloned.

The ASSERT may be triggered in CompositeEditCommand::appendNode
when the fallback content of an OBJECT element is cloned because
the canHaveChildrenForEditing is not reliable for OBJECTs until
their render object is created. This issue is fixed by ignoring
the return value of canHaveChildrenForEditing when an OBJECT is
created.

BUG=399628
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187132

[zherczeg, 2014-11-18 08:48:51]

zherczeg.u-szeged@partner.samsung.com changed reviewers:
+ tkent@chromium.org, yutak@chromium.org

[Yuta Kitamura, 2014-11-18 10:38:03]

yutak@chromium.org changed reviewers:
+ yosin@chromium.org
- tkent@chromium.org

[Yuta Kitamura, 2014-11-18 10:38:03]

-tkent, +yosin

[yosin_UTC9, 2014-11-20 01:26:42]

https://codereview.chromium.org/698113005/diff/20001/LayoutTests/editing/exec...
File LayoutTests/editing/execCommand/crash-object-cloning.html (right):

https://codereview.chromium.org/698113005/diff/20001/LayoutTests/editing/exec...
LayoutTests/editing/execCommand/crash-object-cloning.html:4: {
nit: Move |{| to end of |if| line.
Basically, please format as C++.

https://codereview.chromium.org/698113005/diff/20001/LayoutTests/editing/exec...
LayoutTests/editing/execCommand/crash-object-cloning.html:6:
testRunner.waitUntilDone();
Please add |window.jsTestAsync = true;|, when we call
|testRunner.waitUntilDone()|.

https://codereview.chromium.org/698113005/diff/20001/LayoutTests/editing/exec...
LayoutTests/editing/execCommand/crash-object-cloning.html:8: window.onload =
function() {
Do we really need to wait "load" event for this test?
How about moving <script> block after </body>?

https://codereview.chromium.org/698113005/diff/20001/LayoutTests/editing/exec...
LayoutTests/editing/execCommand/crash-object-cloning.html:9:
document.execCommand("SelectAll",false);
nit: You don't need to have |,false|.

https://codereview.chromium.org/698113005/diff/20001/LayoutTests/editing/exec...
LayoutTests/editing/execCommand/crash-object-cloning.html:10:
document.execCommand("Indent", false);
nit: You don't need to have |,false|.

https://codereview.chromium.org/698113005/diff/20001/Source/core/editing/Comp...
File Source/core/editing/CompositeEditCommand.cpp (right):

https://codereview.chromium.org/698113005/diff/20001/Source/core/editing/Comp...
Source/core/editing/CompositeEditCommand.cpp:365: || (parent->isElementNode() &&
toElement(parent.get())->tagQName() == objectTag));
Could you add a comment why we check OBJECT element?

Note: We have OBJECT element here if |start| or |end| position are in OBJECT's
fallback content in |CompositeEditCommand::cloneParagraphUnderNewElement()|.

[yosin_UTC9, 2014-11-20 01:31:00]

Please describe about this patch rather than root cause, e.g.
Change CompositeEditiCommand::appendNode blah blah ...

[zherczeg, 2014-11-20 13:43:19]

Thank you for the review. I tried to address all of your concerns and uploaded a
new patch.

[zherczeg, 2014-11-24 07:43:50]

On 2014/11/20 13:43:19, zherczeg wrote:
> Thank you for the review. I tried to address all of your concerns and uploaded
a
> new patch.

Could you review this patch?

[yosin_UTC9, 2014-11-25 01:07:32]

code changes and test script are looks fine.
Please update description to explain changes in
|CompositeEditCommand::appendNode()| rather than |useFallbackContent()|.

[zherczeg, 2014-11-25 08:26:25]

On 2014/11/25 01:07:32, Yosi_UTC9 wrote:
> code changes and test script are looks fine.
> Please update description to explain changes in
> |CompositeEditCommand::appendNode()| rather than |useFallbackContent()|.

How can I change the description here? The current description of the patch is:

------------------------------------------

The ASSERT in appendNode() should not fire when OBJECTS are cloned.

The ASSERT may be triggered in CompositeEditCommand::appendNode
when the fallback content of an OBJECT element is cloned because
the canHaveChildrenForEditing is not reliable for OBJECTs until
their render object is created. This issue is fixed by ignoring
the return value of canHaveChildrenForEditing when an OBJECT is
created.

BUG=399628

------------------------------------------

Unfortunately the description of the bug report is not updated. I have no idea
why. I am a new here.

[yosin_UTC9, 2014-11-26 01:00:52]

On 2014/11/25 08:26:25, zherczeg wrote:
> On 2014/11/25 01:07:32, Yosi_UTC9 wrote:
> > code changes and test script are looks fine.
> > Please update description to explain changes in
> > |CompositeEditCommand::appendNode()| rather than |useFallbackContent()|.
> 
> How can I change the description here? The current description of the patch
is:
> 
> ------------------------------------------
> 
> The ASSERT in appendNode() should not fire when OBJECTS are cloned.
> 
> The ASSERT may be triggered in CompositeEditCommand::appendNode
> when the fallback content of an OBJECT element is cloned because
> the canHaveChildrenForEditing is not reliable for OBJECTs until
> their render object is created. This issue is fixed by ignoring
> the return value of canHaveChildrenForEditing when an OBJECT is
> created.
> 
> BUG=399628
> 
> ------------------------------------------
> 
> Unfortunately the description of the bug report is not updated. I have no idea
> why. I am a new here.

You can change patch description and subject by "Edit Issue" link in left side
navigation.

[zherczeg, 2014-11-26 07:42:14]

> You can change patch description and subject by "Edit Issue" link in left side
> navigation.

Thank you. I did it. Is there anything else?

[yosin_UTC9, 2014-11-27 01:13:19]

https://codereview.chromium.org/698113005/diff/40001/LayoutTests/editing/exec...
File LayoutTests/editing/execCommand/crash-object-cloning.html (right):

https://codereview.chromium.org/698113005/diff/40001/LayoutTests/editing/exec...
LayoutTests/editing/execCommand/crash-object-cloning.html:5:
testRunner.dumpAsText();
Could you call |testRunner.waitUntilDone()| and call |testRunner.notifyDone()|
in "load" event handler.

I think test expectation should have indented "abcd" from OBJECT's fallback
contents.

[zherczeg, 2014-11-27 12:15:55]

> I think test expectation should have indented "abcd" from OBJECT's fallback
> contents.

Good point. I made the test async. Even the setTimout is needed.

Anything else?

[zherczeg, 2014-12-02 07:31:40]

> Anything else?

Please review this patch.

[yosin_UTC9, 2014-12-03 01:10:33]

https://codereview.chromium.org/698113005/diff/60001/LayoutTests/editing/exec...
File LayoutTests/editing/execCommand/crash-object-cloning.html (right):

https://codereview.chromium.org/698113005/diff/60001/LayoutTests/editing/exec...
LayoutTests/editing/execCommand/crash-object-cloning.html:14:
setTimeout(function() { testRunner.notifyDone(); }, 0);
Why do we need to call |notifyDone()| in |setTimeout()|?
|ASSERT()| in |appendNode| should be happened during "Indent" command.

[zherczeg, 2014-12-03 07:57:57]

> setTimeout(function() { testRunner.notifyDone(); }, 0);
> Why do we need to call |notifyDone()| in |setTimeout()|?
> |ASSERT()| in |appendNode| should be happened during "Indent" command.

I don't know the internals of the system, but if you remove setTimeout, the test
does not work anymore. I suspect the effect of the change requires a deferred
relayout.

[zherczeg, 2014-12-05 11:12:20]

Ping Yosi_UTC9

[yosin_UTC9, 2014-12-08 02:27:38]

On 2014/12/03 07:57:57, zherczeg wrote:
> > setTimeout(function() { testRunner.notifyDone(); }, 0);
> > Why do we need to call |notifyDone()| in |setTimeout()|?
> > |ASSERT()| in |appendNode| should be happened during "Indent" command.
> 
> I don't know the internals of the system, but if you remove setTimeout, the
test
> does not work anymore. I suspect the effect of the change requires a deferred
> relayout.

What do you mean "does not work"? Timeout?
It is hard to believe we need to call |testRunner.notifyDone()| in timeout
handler.

[zherczeg, 2014-12-08 11:36:47]

> What do you mean "does not work"? Timeout?
> It is hard to believe we need to call |testRunner.notifyDone()| in timeout
> handler.

If I remove the setTimeout, "This test passes if it doesn't crash" does not
appear in the result. Instead, an empty file is created. You are probably right,
that editing itself does not require setTimeout, but I suspect the effect of
this change require it.

Anyway I checked it again and the result indeed disappears without setTimeout.
If this is a bug, it should be fixed in a separate patch.

[yosin_UTC9, 2014-12-09 03:42:07]

On 2014/12/08 11:36:47, zherczeg wrote:
> > What do you mean "does not work"? Timeout?
> > It is hard to believe we need to call |testRunner.notifyDone()| in timeout
> > handler.
> 
> If I remove the setTimeout, "This test passes if it doesn't crash" does not
> appear in the result. Instead, an empty file is created. You are probably
right,
> that editing itself does not require setTimeout, but I suspect the effect of
> this change require it.
> 
> Anyway I checked it again and the result indeed disappears without setTimeout.
> If this is a bug, it should be fixed in a separate patch.

How about adding output after executing "Indent" command? 
Like:
document.execCommand("Indent");
document.body.innerHTML = "The test passses if it doesn't crash"
...
<object>
Fallback text
</object>

It seems "Indent" command make fallback text to hidden.

[zherczeg, 2014-12-09 12:01:44]

> How about adding output after executing "Indent" command? 
> Like:
> document.execCommand("Indent");
> document.body.innerHTML = "The test passses if it doesn't crash"

This seems work. I tested with and without the patch. Patch updated.

Thank you for all comments.

[yosin_UTC9, 2014-12-10 00:45:53]

lgtm
Thanks!

[Yuta Kitamura, 2014-12-10 09:57:32]

lgtm

[zherczeg, 2014-12-15 06:43:30]

The CQ bit was checked by zherczeg.u-szeged@partner.samsung.com

[commit-bot: I haz the power, 2014-12-15 06:44:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/698113005/80001

[commit-bot: I haz the power, 2014-12-15 08:37:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-15 08:37:37]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/41712)

[zherczeg, 2014-12-15 10:47:41]

>   win_blink_rel on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/41712)

New failures:
media/encrypted-media/encrypted-media-lifetime-mediakeysession-release.html

I don't think it is related. Retry landing the patch.

[zherczeg, 2014-12-15 10:47:49]

The CQ bit was checked by zherczeg.u-szeged@partner.samsung.com

[commit-bot: I haz the power, 2014-12-15 10:48:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/698113005/80001

[commit-bot: I haz the power, 2014-12-15 11:26:09]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187132