Bug 48793 – Dragging and dropping into an empty document crashes WebKit

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Daniel Cheng]

elementUnderMouse() in DragController.cpp can return null if the document is empty (see test.xhtml attached to crbug.com/61182).

[Daniel Cheng]

Created attachment 72580 [details]
Patch

[Tony Chang]

Comment on attachment 72580 [details]
Patch

Can you make a layout test for this?

[Daniel Cheng]

Comment on attachment 72580 [details]
Patch

R- while I work on layout test.

[Daniel Cheng]

This one seems to require an empty top-level document to repro. Given the current testing infrastructure, I do not think it is possible to write a layout test. I can attach a manual test to the bug if you want.

[Tony Chang]

Sure, let's just go for a manual test.

[Daniel Cheng]

Created attachment 73304 [details]
Patch

[Tony Chang]

Comment on attachment 73304 [details]
Patch

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

> WebCore/ChangeLog:13
> +        * manual-tests/drop-in-empty-doc2.xhtml: Added.

drop-in-empty-doc2.xhtml should go in manual-tests/resources/.

> WebCore/page/DragController.cpp:271
> +// This can return null if an empty document is loaded.

Is this new or have we had this bug for a long time?  It seems odd that this is suddenly not working.  Do you know which change regressed this?

I'm worried we're papering over the crash when the fix is elsewhere.

[Daniel Cheng]

(In reply to comment #7)
> (From update of attachment 73304 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73304&action=review
> 
> > WebCore/ChangeLog:13
> > +        * manual-tests/drop-in-empty-doc2.xhtml: Added.
> 
> drop-in-empty-doc2.xhtml should go in manual-tests/resources/.
> 
> > WebCore/page/DragController.cpp:271
> > +// This can return null if an empty document is loaded.
> 
> Is this new or have we had this bug for a long time?  It seems odd that this is suddenly not working.  Do you know which change regressed this?
> 
> I'm worried we're papering over the crash when the fix is elsewhere.

I think we've had this bug for a long time (it crashes Chrome 7 as well).

[Ryosuke Niwa]

Crash is P1.

[Tony Chang]

Comment on attachment 73304 [details]
Patch

Sorry, I forgot about this.  I'm going to do some testing in Safari before landing, so removing cq flag.

[Tony Chang]

Committed r73276: <http://trac.webkit.org/changeset/73276>

[Tony Chang]

(In reply to comment #11)
> Committed r73276: <http://trac.webkit.org/changeset/73276>

I verified that this crashes on both Safari 5.0.x (mac) and Safari with WebKit ToT.  The patch fixes it.  I also moved the .xhtml test file into the resources subdir.