Bug 159288

Summary: Prevent crash when attempting to copy an image
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebKit2Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, bfulgham, commit-queue, enrica, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: iPhone / iPad   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=159366
https://bugs.webkit.org/show_bug.cgi?id=159367
Attachments:
Description Flags
Patch
none
Manual test case none

Description Brent Fulgham 2016-06-29 18:35:31 PDT 
The WebProcess crashes under certain circumstances when you attempt to copy and image in iOS.

1. Select an accessory like lightning cable. http://www.apple.com/shop/product/MD819/lightning-to-usb-cable-2-m?fnode=37
2. Tap and hold the image on the right > Copy
3. WebKit crashes. Image is not copied.
Comment 1 Brent Fulgham 2016-06-29 18:37:42 PDT 
This is due to a null-pointer dereference in WebPage::performActionOnElement.

In this test case, the HTMLImageElement's renderer does not have a valid cached image. We ask the nullptr for its URL, and the process crashes.

This could be avoided by a nullptr check, but the method we pass the URL to doesn't even use it! So we can avoid the whole issue by simply passing a default URL that can be just as easily ignored as a fully-formed URL (at less cost!).
Comment 2 Brent Fulgham 2016-06-29 18:37:57 PDT 
<rdar://problem/23507828>
Comment 3 Brent Fulgham 2016-06-29 18:41:04 PDT 
Created attachment 282402 [details]
Patch
Comment 4 Brady Eidson 2016-06-29 21:38:04 PDT 
Comment on attachment 282402 [details]
Patch

Test?
Comment 5 Brent Fulgham 2016-06-30 14:48:11 PDT 
Note: This crash only reproduces if you LONG PRESS to display the Action Sheet, and then use the "Copy" action sheet option.
Comment 6 Brent Fulgham 2016-07-01 09:37:48 PDT 
Created attachment 282556 [details]
Manual test case

I cannot create an API test or LayoutTest for this, because our iOS testing infrastructure does not support controlling action sheets or simulating touch events.

Test:
1. Unzip the attached "Image_Copy_Crash.zip" someplace you can server from a web server.
2. Navigate to the "image_copy_crash.html" file in iOS.
3. Long-press on the colored region of the screen.
4. Select 'Copy'.

If you do not crash, the test passes.
Comment 7 Brent Fulgham 2016-07-01 13:27:34 PDT 
Neither WebKitTestRunner nor TestWebKitAPI support driving the UI interaction needed to trigger this bug.
Comment 8 Brent Fulgham 2016-07-01 13:33:02 PDT 
I have filed Bug 159366 to expand our testing infrastructure to allow automated tests that exercise the UI interactions required to reproduce this crash.
Comment 9 Brent Fulgham 2016-07-01 13:47:57 PDT 
Note that this crash revealed that specific markup can cause the cachedImage value to be null. I filled Bug 159367 to investigate this underlying bug.
Comment 10 WebKit Commit Bot 2016-07-01 13:52:57 PDT 
Comment on attachment 282402 [details]
Patch

Clearing flags on attachment: 282402

Committed r202754: <http://trac.webkit.org/changeset/202754>
Comment 11 WebKit Commit Bot 2016-07-01 13:53:01 PDT 
All reviewed patches have been landed.  Closing bug.