Bug 154059 - WebFrame _stringByEvaluatingJavaScriptFromString:forceUserGesture: should assert that it is being called from the "main" thread.
Summary: WebFrame _stringByEvaluatingJavaScriptFromString:forceUserGesture: should ass...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-02-09 18:54 PST by Mark Lam
Modified: 2016-02-12 16:15 PST (History)
8 users (show)

See Also:


Attachments
proposed patch. (1.47 KB, patch)
2016-02-10 10:57 PST, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2016-02-09 18:54:25 PST
This makes it so that misbehaving clients which call it (indirectly) from another thread (not the main thread) will fail faster.  Otherwise, we get potential memory corruption that results in strange crashes elsewhere later.
Comment 1 Radar WebKit Bug Importer 2016-02-10 10:38:42 PST
<rdar://problem/24590120>
Comment 2 Mark Lam 2016-02-10 10:57:12 PST
Created attachment 271005 [details]
proposed patch.
Comment 3 Geoffrey Garen 2016-02-10 13:26:37 PST
Comment on attachment 271005 [details]
proposed patch.

r=me
Comment 4 WebKit Commit Bot 2016-02-10 14:17:00 PST
Comment on attachment 271005 [details]
proposed patch.

Clearing flags on attachment: 271005

Committed r196395: <http://trac.webkit.org/changeset/196395>
Comment 5 WebKit Commit Bot 2016-02-10 14:17:02 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Darin Adler 2016-02-11 07:26:32 PST
We typically use Objective-C exceptions rather than RELEASE_ASSERT for this kind of thread restriction. These are easier for developers to understand than a crash.
Comment 7 Darin Adler 2016-02-11 08:33:58 PST
Our previous work on this uses these functions:

WebCoreThreadViolationCheckRoundOne
WebCoreThreadViolationCheckRoundTwo

from the header ThreadCheck.h
Comment 8 Darin Adler 2016-02-11 08:40:17 PST
Instead, or possibly in addition to, this RELEASE_ASSERT, we should add WebCoreThreadViolationCheckRoundTwo to the public API entry point -[WebView stringByEvaluatingJavaScriptFromString:] or come up with a WebCoreThreadViolationCheckRoundThree. And possibly add it to the internal bottleneck method as well.

A lot of thought went into how WebCoreThreadViolationCheck works, and it’s designed for just this sort of situation.

On the other hand, it’s not surprising we have forgotten about this since that work was done 7 years ago!

Bug 22976 was when we created all of this.
Comment 9 Mark Lam 2016-02-12 16:15:44 PST
(In reply to comment #8)
> A lot of thought went into how WebCoreThreadViolationCheck works, and it’s
> designed for just this sort of situation.

I've added thread violation checks to WebView's public APIs in https://bugs.webkit.org/show_bug.cgi?id=154183.