Bug 110350

Summary: ASSERT(upstreamStart.deprecatedNode()->isDescendantOf(enclosingBlock(upstreamEnd.deprecatedNode()))) in CompositeEditCommand::moveParagraphContentsToNewBlockIfNecessary
Product: WebKit Reporter: Renata Hodovan <rhodovan.u-szeged>
Component: HTML EditingAssignee: Éva Balázsfalvi <evab.u-szeged>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: bfulgham, buildbot, darin, evab.u-szeged, jiewen_tan, ossy, rniwa, tkent
Priority: P2 Keywords: BlinkMergeCandidate
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=151288
Bug Depends on:    
Bug Blocks: 116980    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2
none
Patch
none
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
none
Patch ossy: review-

Description Renata Hodovan 2013-02-20 08:21:14 PST
Running the test below in Debug we got an assertion failure:

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff42a85bc in WebCore::CompositeEditCommand::moveParagraphContentsToNewBlockIfNecessary (
    this=0x848100, pos=...) at /home/reni/Data/REPOS/webkit/Source/WebCore/editing/CompositeEditCommand.cpp:947
947	            ASSERT(upstreamStart.deprecatedNode()->isDescendantOf(enclosingBlock(upstreamEnd.deprecatedNode())));



The test:


<html>
<body>
	<ul style="height: 20px;"></ul>
	<div>
		<div></div>
		<a target="foo">more</a>
	</div>
	<script>
		document.designMode = "on";
		document.execCommand("SelectAll");
		document.execCommand("CreateLink", 0, 'foo');
		document.execCommand("SelectAll");
		document.execCommand("JustifyCenter");
	</script>
</body>
</html>
Comment 1 Éva Balázsfalvi 2014-06-23 05:11:51 PDT
Created attachment 233598 [details]
Patch
Comment 2 Build Bot 2014-06-23 06:39:36 PDT
Comment on attachment 233598 [details]
Patch

Attachment 233598 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4992311616339968

New failing tests:
editing/execCommand/justify-center-empty-list-crash.html
Comment 3 Build Bot 2014-06-23 06:39:38 PDT
Created attachment 233601 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 4 Build Bot 2014-06-23 07:35:30 PDT
Comment on attachment 233598 [details]
Patch

Attachment 233598 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6261390700445696

New failing tests:
editing/execCommand/justify-center-empty-list-crash.html
Comment 5 Build Bot 2014-06-23 07:35:33 PDT
Created attachment 233605 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 6 Build Bot 2014-06-23 10:15:42 PDT
Comment on attachment 233598 [details]
Patch

Attachment 233598 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4728393929064448

New failing tests:
editing/execCommand/justify-center-empty-list-crash.html
media/W3C/video/networkState/networkState_during_loadstart.html
Comment 7 Build Bot 2014-06-23 10:15:45 PDT
Created attachment 233615 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 8 Csaba Osztrogonác 2014-06-24 03:05:14 PDT
Comment on attachment 233598 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=233598&action=review

> LayoutTests/editing/execCommand/justify-center-empty-list-crash.html:12
> +    document.body.textContent = 'PASS if Blink doesn\'t crash.';

Blink?
Comment 9 Éva Balázsfalvi 2014-06-24 05:10:07 PDT
Created attachment 233695 [details]
Patch
Comment 10 Build Bot 2014-06-24 08:22:15 PDT
Comment on attachment 233695 [details]
Patch

Attachment 233695 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6315280166813696

New failing tests:
media/track/add-and-remove-track.html
fullscreen/video-cursor-auto-hide.html
Comment 11 Build Bot 2014-06-24 08:22:17 PDT
Created attachment 233706 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 12 Éva Balázsfalvi 2014-07-23 04:24:06 PDT
Could someone review this patch for me? Thank you!
Comment 13 Éva Balázsfalvi 2014-08-29 02:33:00 PDT
Comment on attachment 233695 [details]
Patch

A review would be really appreciated. Thanks in advance.
Comment 14 Darin Adler 2014-08-29 09:17:37 PDT
Comment on attachment 233695 [details]
Patch

This patch seems kind of strange. It removes an assertion, without any explanation of why except that Chromium already removed it. We need to explain changes. It’s obvious that removing an assertion will make the crash intentionally caused by the assertion go away. What’s not obvious is whether there is some kind of problem left behind. Some editing export should review this and explain why no assertion is needed here.
Comment 15 Csaba Osztrogonác 2014-11-18 02:14:17 PST
(In reply to comment #14)
> Comment on attachment 233695 [details]
> Patch
> 
> This patch seems kind of strange. It removes an assertion, without any
> explanation of why except that Chromium already removed it. We need to
> explain changes. It’s obvious that removing an assertion will make the crash
> intentionally caused by the assertion go away. What’s not obvious is whether
> there is some kind of problem left behind. Some editing export should review
> this and explain why no assertion is needed here.

https://src.chromium.org/viewvc/blink?revision=175118&view=revision explained
that this assert was based on false assumption and added a test case where
this assert fires incorrectly.
Comment 16 Éva Balázsfalvi 2014-11-18 04:08:53 PST
Created attachment 241779 [details]
Patch
Comment 17 Éva Balázsfalvi 2014-11-21 05:33:11 PST
Ping?
Comment 18 Csaba Osztrogonác 2016-03-10 07:42:39 PST
Comment on attachment 241779 [details]
Patch

This assertion was removed by bug151288. Could you confirm if it is dup of it?
Comment 19 Brent Fulgham 2016-08-03 11:04:23 PDT
(In reply to comment #18)
> Comment on attachment 241779 [details]
> Patch
> 
> This assertion was removed by bug151288. Could you confirm if it is dup of
> it?

This is a duplicate. The test case from the reporter does not assert any longer.
Comment 20 Brent Fulgham 2016-08-03 11:04:38 PDT

*** This bug has been marked as a duplicate of bug 151288 ***