Bug 165157 - Web Inspector: Clicking on link in Web Inspector can cause UIProcess to crash
Summary: Web Inspector: Clicking on link in Web Inspector can cause UIProcess to crash
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-11-29 14:00 PST by Joseph Pecoraro
Modified: 2016-11-30 11:48 PST (History)
7 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (22.96 KB, patch)
2016-11-29 14:05 PST, Joseph Pecoraro
bburg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2016-11-29 14:00:07 PST
Summary:
Clicking on link in Web Inspector can cause UIProcess to crash

Test:
<a href="/http://example.com">Link</a>

Steps to Reproduce:
1. Inspect the link on the test page on a file:/// domain
2. Click on the "/http://example.com" link in the Element's DOM Tree
  => ASSERT / Crash

Crash:
Exception Type:        EXC_BREAKPOINT (SIGTRAP)
Exception Codes:       0x0000000000000002, 0x0000000000000000
Exception Note:        EXC_CORPSE_NOTIFY

Application Specific Information:
Invalid WebProcess IPC Message:
Message "WebPageProxy.DecidePolicyForNavigationAction"

Application Specific Signatures:
Invalid Web Process IPC Message ID "WebPageProxy.DecidePolicyForNavigationAction"

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.Safari.framework    	0x00000001051d2bc7 invalidMessageFunction(OpaqueWKString const*) + 293 (AppController.mm:743)
1   com.apple.WebKit              	0x00000001081a46ca WebKit::WebProcessPool::didReceiveInvalidMessage(IPC::StringReference const&, IPC::StringReference const&) + 162 (Ref.h:58)
2   com.apple.WebKit              	0x00000001081b0f95 WebKit::WebProcessProxy::didReceiveInvalidMessage(IPC::Connection&, IPC::StringReference, IPC::StringReference) + 185 (WebProcessProxy.cpp:537)
3   com.apple.WebKit              	0x0000000107f7e425 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 211 (Connection.cpp:936)
4   com.apple.WebKit              	0x0000000107f81005 IPC::Connection::dispatchOneMessage() + 175 (memory:2722)

Notes:
Here we have a link like this:

    <a href="/http://example.com">Link</a>

Which should resolve to an absolute URL like:

    "<document base url>/http://example.com"

However Web Inspector thinks it is a complete URL:

    "/http://example.com"

Which when put into a <a> inside Web Inspector becomes:

    "file:///http://example.com"

When clicked in Web Inspector, Web Inspector tries to open a new tab for that URL. The UI Process sanity checks the URL and does not allow file URLs that aren't already allowed. Its defense is to crash.

The issue here was the absolute URL that Web Inspector created was incorrect because it incorrectly thought "/http://example.com" was a complete URL. The defense mechanism seems fine.
Comment 1 Joseph Pecoraro 2016-11-29 14:00:15 PST
<rdar://problem/27896562>
Comment 2 Joseph Pecoraro 2016-11-29 14:05:52 PST
Created attachment 295635 [details]
[PATCH] Proposed Fix
Comment 3 BJ Burg 2016-11-30 09:34:41 PST
Comment on attachment 295635 [details]
[PATCH] Proposed Fix

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

r=me

> LayoutTests/inspector/unit-tests/url-utilities.html:196
> +            // FIXME: <https://webkit.org/b/165155> Web Inspector: Use URL constructor to better handle all kinds of URLs

Do we want to leave this stuff in the test case? If it is currently broken in these cases maybe we can rebaseline the results to pass later.
Comment 4 Joseph Pecoraro 2016-11-30 11:01:39 PST
(In reply to comment #3)
> Comment on attachment 295635 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295635&action=review
> 
> r=me
> 
> > LayoutTests/inspector/unit-tests/url-utilities.html:196
> > +            // FIXME: <https://webkit.org/b/165155> Web Inspector: Use URL constructor to better handle all kinds of URLs
> 
> Do we want to leave this stuff in the test case? If it is currently broken
> in these cases maybe we can rebaseline the results to pass later.

Leave them in failing at the moment with a FIXME? Yeah, that sounds good. I'll do it.
Comment 5 Joseph Pecoraro 2016-11-30 11:48:46 PST
<https://trac.webkit.org/changeset/209143>