Bug 201449 - Structure::storedPrototype() and storedPrototypeObject() should assert with isCompilationThread(), not !isMainThread().
Summary: Structure::storedPrototype() and storedPrototypeObject() should assert with i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-09-03 23:25 PDT by Mark Lam
Modified: 2019-09-04 14:14 PDT (History)
7 users (show)

See Also:


Attachments
proposed patch. (1.95 KB, patch)
2019-09-03 23:32 PDT, Mark Lam
ysuzuki: review+
Details | Formatted Diff | Diff
patch for landing. (2.16 KB, patch)
2019-09-04 11:47 PDT, 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 2019-09-03 23:25:37 PDT
This is a follow up to https://bugs.webkit.org/show_bug.cgi?id=201281.
Comment 1 Mark Lam 2019-09-03 23:32:42 PDT
Created attachment 377959 [details]
proposed patch.

Let's try this on the EWS.
Comment 2 Yusuke Suzuki 2019-09-04 02:22:57 PDT
Comment on attachment 377959 [details]
proposed patch.

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

r=me

> Source/JavaScriptCore/runtime/StructureInlines.h:111
> +    ASSERT(isCompilationThread() || object->structure() == this);

Is it possible that the GC thread accesses to this functions? If so, we should allow GC thread too (I think we have an check like isCompilationThread). If not, this check is fine.

> Source/JavaScriptCore/runtime/StructureInlines.h:119
> +    ASSERT(isCompilationThread() || object->structure() == this);

Ditto.
Comment 3 Mark Lam 2019-09-04 11:25:31 PDT
Comment on attachment 377959 [details]
proposed patch.

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

>> Source/JavaScriptCore/runtime/StructureInlines.h:111
>> +    ASSERT(isCompilationThread() || object->structure() == this);
> 
> Is it possible that the GC thread accesses to this functions? If so, we should allow GC thread too (I think we have an check like isCompilationThread). If not, this check is fine.

AFAICT from grepping the code and from running tests, this is not called from GC threads.  However, there is a Thread::mayBeGCThread() available.  To better communicate our intent here, I'll add Thread::mayBeGCThread() to the assert clauses.
Comment 4 Mark Lam 2019-09-04 11:47:38 PDT
Created attachment 377993 [details]
patch for landing.
Comment 5 Mark Lam 2019-09-04 14:13:28 PDT
Thanks for the review.  Landed in r249499: <http://trac.webkit.org/r249499>.
Comment 6 Radar WebKit Bug Importer 2019-09-04 14:14:16 PDT
<rdar://problem/55037141>