Bug 146009

Summary: [iOS] Crash long pressing on <input type=file>
Product: WebKit Reporter: Jon Honeycutt <jhoneycutt>
Component: HTML EditingAssignee: Jon Honeycutt <jhoneycutt>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, kangil.han, rniwa
Priority: P2 Keywords: InRadar, PlatformOnly
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch rniwa: review+

Description Jon Honeycutt 2015-06-16 00:01:36 PDT
The web process crashes when trying to long press on an <input type=file> control.

<rdar://problem/21234453>
Comment 1 Jon Honeycutt 2015-06-16 00:07:01 PDT
Created attachment 254941 [details]
Patch
Comment 2 Ryosuke Niwa 2015-06-16 02:10:32 PDT
Comment on attachment 254941 [details]
Patch

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

> Source/WebCore/ChangeLog:5
> +
> +        <https://bugs.webkit.org/show_bug.cgi?id=146009>

We don't usually place a blank line between the bug title and the bugzilla URL.
Also, we don't normally warp a bugzilla URL with < & > unlike a rdar URL.

> ChangeLog:5
> +
> +        <https://bugs.webkit.org/show_bug.cgi?id=146009>

Ditto.

> ChangeLog:10
> +        * ManualTests/ios/long-press-input-type-file-crash.html: Added.

It's sad we can't automate this :(

> ManualTests/ios/long-press-input-type-file-crash.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Why don't we use <!DOCTYPE html> instead?
Comment 3 Darin Adler 2015-06-16 10:50:05 PDT
Comment on attachment 254941 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        * dom/Position.cpp:
> +        (WebCore::Position::atStartOfTree):
> +        (WebCore::Position::atEndOfTree):
> +        Null check the container node before passing it to findParent().

If findParent doesn’t work on null, it should take a reference rather than a pointer. Or we could put the check for null inside findParent.
Comment 4 Jon Honeycutt 2015-06-16 14:34:05 PDT
(In reply to comment #2)
> Comment on attachment 254941 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254941&action=review
> 
> > Source/WebCore/ChangeLog:5
> > +
> > +        <https://bugs.webkit.org/show_bug.cgi?id=146009>
> 
> We don't usually place a blank line between the bug title and the bugzilla
> URL.
> Also, we don't normally warp a bugzilla URL with < & > unlike a rdar URL.
> 
> > ChangeLog:5
> > +
> > +        <https://bugs.webkit.org/show_bug.cgi?id=146009>
> 
> Ditto.

Fixed.

> 
> > ChangeLog:10
> > +        * ManualTests/ios/long-press-input-type-file-crash.html: Added.
> 
> It's sad we can't automate this :(
> 
> > ManualTests/ios/long-press-input-type-file-crash.html:1
> > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
> 
> Why don't we use <!DOCTYPE html> instead?

Fixed.

Thanks for the review!
Comment 5 Jon Honeycutt 2015-06-16 14:35:18 PDT
(In reply to comment #3)
> Comment on attachment 254941 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254941&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        * dom/Position.cpp:
> > +        (WebCore::Position::atStartOfTree):
> > +        (WebCore::Position::atEndOfTree):
> > +        Null check the container node before passing it to findParent().
> 
> If findParent doesn’t work on null, it should take a reference rather than a
> pointer. Or we could put the check for null inside findParent.

I’m going to land this as-is and make a follow-up patch that does one of the two.
Comment 6 Jon Honeycutt 2015-06-16 14:40:36 PDT
Committed r185613: <http://trac.webkit.org/changeset/185613>