Bug 130095 - [iOS WebKit2] Crash when trying to select inside a video element with longpress.
Summary: [iOS WebKit2] Crash when trying to select inside a video element with longpress.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Enrica Casucci
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-03-11 15:55 PDT by Enrica Casucci
Modified: 2014-03-12 16:24 PDT (History)
0 users

See Also:


Attachments
Patch (2.73 KB, patch)
2014-03-11 15:59 PDT, Enrica Casucci
no flags Details | Formatted Diff | Diff
Patch2 (3.09 KB, patch)
2014-03-11 18:01 PDT, Enrica Casucci
no flags Details | Formatted Diff | Diff
Patch3 (2.58 KB, patch)
2014-03-11 18:02 PDT, Enrica Casucci
benjamin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Enrica Casucci 2014-03-11 15:55:38 PDT
Repro steps:

Launch Safari (WK2)
Nevigate to www.devour.com and start playing a video.
Tap and hold inside the video element.

Expected
No crash and no selection.

Actual
Attempt to perform block selection and crash.

<rdar://problem/16294534>
Comment 1 Enrica Casucci 2014-03-11 15:59:52 PDT
Created attachment 226440 [details]
Patch
Comment 2 Enrica Casucci 2014-03-11 18:01:03 PDT
Created attachment 226454 [details]
Patch2
Comment 3 Enrica Casucci 2014-03-11 18:02:49 PDT
Created attachment 226455 [details]
Patch3
Comment 4 Benjamin Poulain 2014-03-11 18:35:42 PDT
Comment on attachment 226455 [details]
Patch3

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

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:562
> -    return range;
> +    return range->collapsed(ASSERT_NO_EXCEPTION) ? nullptr : range;

Is this related?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1492
> +        if (isHTMLVideoElement(hitNode) || isHTMLAudioElement(hitNode))
> +            return;

Let's make this explicit.

What about adding a flag "canStartSelection" on InteractionInformationAtPosition?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1684
> -    m_assistedNode = node;
>      if (node->hasTagName(WebCore::HTMLNames::selectTag) || node->hasTagName(WebCore::HTMLNames::inputTag) || node->hasTagName(WebCore::HTMLNames::textareaTag) || node->hasEditableStyle()) {
> +        m_assistedNode = node;
>          AssistedNodeInformation information;

Should we be able to pause the video from a physical keyboard?
Comment 5 Enrica Casucci 2014-03-12 09:59:25 PDT
Comment on attachment 226455 [details]
Patch3

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

>> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:562
>> +    return range->collapsed(ASSERT_NO_EXCEPTION) ? nullptr : range;
> 
> Is this related?

Not strictly to the crash, but it is part of improving the block selection experience.  In some cases the created range could be collapsed and we don't want to use it for block selection (since it represents a caret selection).

>> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1492
>> +            return;
> 
> Let's make this explicit.
> 
> What about adding a flag "canStartSelection" on InteractionInformationAtPosition?

I'll remove this for this patch. I want to make a separate patch that improves blockselection and excludes large blocks. I'll make it part of it.

>> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1684
>>          AssistedNodeInformation information;
> 
> Should we be able to pause the video from a physical keyboard?

I don't know. I will test it. I believe that if this is possible in WebKit1 it is handled at the plugin level.
Comment 6 Benjamin Poulain 2014-03-12 13:33:38 PDT
Comment on attachment 226455 [details]
Patch3

Okay then.
Comment 7 Enrica Casucci 2014-03-12 16:24:22 PDT
Committed revision 165506.