Bug 163986 – JSC should support SharedArrayBuffer

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 292820 [details]
it begins

[Filip Pizlo]

Comment on attachment 292820 [details]
it begins

It's nowhere near ready for review.

[WebKit Commit Bot]

Attachment 292820 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:36:  Missing space inside { }.  [whitespace/braces] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 292841 [details]
getting closer

[Filip Pizlo]

Relevant: https://github.com/tc39/ecmascript_sharedmem/issues/38#issuecomment-163048837

I think that we want to make it harder to unwrap an ArrayBuffer if you haven't made a statement about whether you're happy sharing.

[Filip Pizlo]

Created attachment 292874 [details]
more

[Filip Pizlo]

Created attachment 292955 [details]
more

[Filip Pizlo]

Created attachment 292994 [details]
a bit more

[Filip Pizlo]

Created attachment 293023 [details]
more

I'm renaming all of the functions that give native code access to a typed array's guts.  For each toBlahTypedArray()/toArrayBuffer() function, we will now have two:

toUnsharedBlah() -> same as the old toBlah() but asserts that the underlying buffer is unshared, or in the case of versions that dynamic cast, returns zero if the underlying buffer is unshared.
toPossiblySharedBlah() -> same as the old toBlah() but may return something that has memory that is aliased with another thread.

[Filip Pizlo]

Created attachment 293028 [details]
rebased patch

[Filip Pizlo]

Created attachment 293094 [details]
more

[Filip Pizlo]

Created attachment 293113 [details]
it's starting to compile

[Filip Pizlo]

Created attachment 293165 [details]
closer to compiling

[Filip Pizlo]

Created attachment 293172 [details]
jsc compiles

[Filip Pizlo]

Created attachment 293181 [details]
making the DOM happy

[Filip Pizlo]

Created attachment 293188 [details]
working through more DOM uses of typed arrays

[Filip Pizlo]

Created attachment 293195 [details]
more of the DOM compiles

[Filip Pizlo]

Created attachment 293273 [details]
more of the DOM compiles

[Filip Pizlo]

Created attachment 293299 [details]
the DOM compiles

[Filip Pizlo]

Created attachment 293307 [details]
it all builds

It builds and SharedArrayBuffer is behind a runtime flag.  I haven't tested it yet.

[Filip Pizlo]

Created attachment 293309 [details]
rebased patch

[WebKit Commit Bot]

Attachment 293309 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:184:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 69 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 293311 [details]
more fixes

[WebKit Commit Bot]

Attachment 293311 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:184:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 70 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 293324 [details]
it seems to work

[Filip Pizlo]

Created attachment 293325 [details]
rebased patch

[WebKit Commit Bot]

Attachment 293325 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:100:  Declaration has space between type name and * in const JSWorkerGlobalScopeBase *thisObject  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:184:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 6 in 93 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 293326 [details]
better changelog

[WebKit Commit Bot]

Attachment 293326 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:100:  Declaration has space between type name and * in const JSWorkerGlobalScopeBase *thisObject  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:184:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 6 in 93 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 293326 [details]
better changelog

Attachment 293326 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2403208

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 293329 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 293326 [details]
better changelog

Attachment 293326 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2403207

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 293330 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 293326 [details]
better changelog

Attachment 293326 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2403205

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 293331 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 293326 [details]
better changelog

Attachment 293326 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2403217

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 293333 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Filip Pizlo]

Ooops the crashes look like a missing null check.

[Filip Pizlo]

Created attachment 293352 [details]
some fixes

toUnsharedArrayBufferView was missing a null check.

[Filip Pizlo]

Created attachment 293353 [details]
rebased

[WebKit Commit Bot]

Attachment 293353 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:101:  Declaration has space between type name and * in const JSWorkerGlobalScopeBase *thisObject  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:184:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 6 in 93 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 293354 [details]
fix gcc build

[WebKit Commit Bot]

Attachment 293354 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:101:  Declaration has space between type name and * in const JSWorkerGlobalScopeBase *thisObject  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:184:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 6 in 93 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 293355 [details]
the patch

[WebKit Commit Bot]

Attachment 293355 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:101:  Declaration has space between type name and * in const JSWorkerGlobalScopeBase *thisObject  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:184:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 6 in 93 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 293356 [details]
the patch

[WebKit Commit Bot]

Attachment 293356 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:101:  Declaration has space between type name and * in const JSWorkerGlobalScopeBase *thisObject  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:184:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 6 in 93 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 293358 [details]
the patch

[WebKit Commit Bot]

Attachment 293358 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:101:  Declaration has space between type name and * in const JSWorkerGlobalScopeBase *thisObject  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:184:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 6 in 93 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Looks like I messed up ArrayBuffer transfer for the non-shared case.

[Build Bot]

Comment on attachment 293358 [details]
the patch

Attachment 293358 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2406561

New failing tests:
js/typed-array-mutated-during-set.html
workers/sab/simple.html
fast/dom/Window/window-postmessage-args.html
fast/canvas/webgl/arraybuffer-transfer-of-control.html
js/dom/global-constructors-attributes-dedicated-worker.html
js/dom/dfg-byteOffset-neuter.html

[Build Bot]

Created attachment 293362 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 293358 [details]
the patch

Attachment 293358 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2406562

New failing tests:
js/dom/global-constructors-attributes.html
workers/sab/simple.html
js/dom/dfg-byteOffset-neuter.html
fast/canvas/webgl/arraybuffer-transfer-of-control.html
js/dom/global-constructors-attributes-dedicated-worker.html
fast/dom/Window/window-postmessage-args.html
js/typed-array-mutated-during-set.html

[Build Bot]

Created attachment 293364 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 293358 [details]
the patch

Attachment 293358 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2406565

New failing tests:
js/typed-array-mutated-during-set.html
workers/sab/simple.html
fast/dom/Window/window-postmessage-args.html
fast/canvas/webgl/arraybuffer-transfer-of-control.html
js/dom/global-constructors-attributes-dedicated-worker.html
js/dom/dfg-byteOffset-neuter.html

[Build Bot]

Created attachment 293366 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 293358 [details]
the patch

Attachment 293358 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2406568

New failing tests:
js/dom/dfg-byteOffset-neuter.html
fast/dom/Window/window-postmessage-args.html
js/dom/global-constructors-attributes-dedicated-worker.html
js/typed-array-mutated-during-set.html
workers/sab/simple.html

[Build Bot]

Created attachment 293367 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Filip Pizlo]

I'm debugging the crashes.  They are subtle.  An array buffer view's vector goes null at some point, don't know why yet.

[Filip Pizlo]

(In reply to comment #59)
> I'm debugging the crashes.  They are subtle.  An array buffer view's vector
> goes null at some point, don't know why yet.

Found it.  ArrayBuffer::transferTo() needs to clear() this.  Failing to do so creates all manner of problems.

Basically, the vector was supposed to be null because of neutering, but our logic for handling neutered array buffers wasn't working right because the ArrayBuffer itself had not been neutered (because the of the missing clear call).

[Filip Pizlo]

Created attachment 293391 [details]
the patch

I fixed the crash in neutering.

[WebKit Commit Bot]

Attachment 293391 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:101:  Declaration has space between type name and * in const JSWorkerGlobalScopeBase *thisObject  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:185:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 6 in 93 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 293391 [details]
the patch

Attachment 293391 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2409241

New failing tests:
js/dom/global-constructors-attributes-dedicated-worker.html
workers/sab/simple.html

[Build Bot]

Created attachment 293392 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 293391 [details]
the patch

Attachment 293391 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2409260

New failing tests:
js/dom/global-constructors-attributes.html
js/dom/global-constructors-attributes-dedicated-worker.html
media/modern-media-controls/volume-support/volume-support-click.html
workers/sab/simple.html

[Build Bot]

Created attachment 293393 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 293391 [details]
the patch

Attachment 293391 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2409254

New failing tests:
js/dom/global-constructors-attributes-dedicated-worker.html
workers/sab/simple.html

[Build Bot]

Created attachment 293394 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 293391 [details]
the patch

Attachment 293391 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2409276

New failing tests:
js/dom/global-constructors-attributes-dedicated-worker.html
workers/sab/simple.html

[Build Bot]

Created attachment 293395 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Filip Pizlo]

Created attachment 293423 [details]
the patch

[WebKit Commit Bot]

Attachment 293423 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:101:  Declaration has space between type name and * in const JSWorkerGlobalScopeBase *thisObject  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:185:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 6 in 95 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 293426 [details]
the patch

[WebKit Commit Bot]

Attachment 293426 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:101:  Declaration has space between type name and * in const JSWorkerGlobalScopeBase *thisObject  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:185:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 6 in 95 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 293426 [details]
the patch

Attachment 293426 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2411984

New failing tests:
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 293432 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Filip Pizlo]

Created attachment 293434 [details]
more tests WIP

[Keith Miller]

Comment on attachment 293426 [details]
the patch

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

Some comments. Mostly looks good. I'll hold off on the final review until the tests are done though.

> Source/JavaScriptCore/ChangeLog:20
> +        There is also a Atomics object in global scope, which exposes sequentiall consistent atomic

typo: a Atomcis => an Atomics. sequentiall => sequentially.

> Source/JavaScriptCore/runtime/ArrayBuffer.cpp:109
> +        ASSERT(policy == DontInitialize);

Is this assert really necessary?

> Source/JavaScriptCore/runtime/ArrayBuffer.h:188
> +    // FIXME: We probably want to scale this by the shared ref count or something.

yikes, this seems unfortunate.

> Source/JavaScriptCore/runtime/AtomicsObject.cpp:322
> +    // FIXME: Stop using std::chrono.

File a bug! :)

[Filip Pizlo]

(In reply to comment #78)
> Comment on attachment 293426 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=293426&action=review
> 
> Some comments. Mostly looks good. I'll hold off on the final review until
> the tests are done though.
> 
> > Source/JavaScriptCore/ChangeLog:20
> > +        There is also a Atomics object in global scope, which exposes sequentiall consistent atomic
> 
> typo: a Atomcis => an Atomics. sequentiall => sequentially.

Fixed.

> 
> > Source/JavaScriptCore/runtime/ArrayBuffer.cpp:109
> > +        ASSERT(policy == DontInitialize);
> 
> Is this assert really necessary?

I think it was there before I moved that code from ArrayBuffer.h

> 
> > Source/JavaScriptCore/runtime/ArrayBuffer.h:188
> > +    // FIXME: We probably want to scale this by the shared ref count or something.
> 
> yikes, this seems unfortunate.
> 
> > Source/JavaScriptCore/runtime/AtomicsObject.cpp:322
> > +    // FIXME: Stop using std::chrono.
> 
> File a bug! :)

I think we have one.  I'll find it.

[JF Bastien]

Comment on attachment 293426 [details]
the patch

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

> Source/JavaScriptCore/ChangeLog:20
> +        There is also a Atomics object in global scope, which exposes sequentiall consistent atomic

Typo "sequential"

> Source/JavaScriptCore/runtime/ArrayBuffer.cpp:95
> +{

Should this set `m_shared = nullptr;` at the beginning? Or assert on it?

> Source/JavaScriptCore/runtime/ArrayBuffer.cpp:101
> +            m_data = nullptr;

It seems like setting `m_sizeInBytes = 0;` on both failure paths would be conservatively good.

> Source/JavaScriptCore/runtime/ArrayBufferSharingMode.h:33
> +    Default,

Why not "Unshared"? That seems more explicit.

> Source/JavaScriptCore/runtime/AtomicsObject.cpp:229
> +    case 3:

case 3 should return false, and case 4 should return true (along with 1 and 2).

> Source/JavaScriptCore/runtime/AtomicsObject.cpp:305
> +    double timeoutInNanoseconds = timeoutInMilliseconds * 1000;

Missing 1000?

> Source/JavaScriptCore/runtime/AtomicsObject.cpp:315
> +    if (timeoutInNanoseconds == timeoutInNanoseconds)

std::isnan

> Source/JavaScriptCore/runtime/AtomicsObject.cpp:316
> +        timeoutInNanoseconds = std::max(0., timeoutInNanoseconds);

This seems to be a spec thing: is -0. a valid thing here?

> Source/WTF/wtf/Atomics.h:-96
> -    ASSERT(bitwise_cast<std::atomic<T>*>(location)->is_lock_free() && "expected lock-free type");

I think you want to keep these asserts.

[Filip Pizlo]

(In reply to comment #80)
> Comment on attachment 293426 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=293426&action=review
> 
> > Source/JavaScriptCore/ChangeLog:20
> > +        There is also a Atomics object in global scope, which exposes sequentiall consistent atomic
> 
> Typo "sequential"

Fixed.

> 
> > Source/JavaScriptCore/runtime/ArrayBuffer.cpp:95
> > +{
> 
> Should this set `m_shared = nullptr;` at the beginning? Or assert on it?

I think it's already certain that this function is only called right after ArrayBufferContents is instantiated.

> 
> > Source/JavaScriptCore/runtime/ArrayBuffer.cpp:101
> > +            m_data = nullptr;
> 
> It seems like setting `m_sizeInBytes = 0;` on both failure paths would be
> conservatively good.

I made it call reset().

> 
> > Source/JavaScriptCore/runtime/ArrayBufferSharingMode.h:33
> > +    Default,
> 
> Why not "Unshared"? That seems more explicit.

To reflect the naming of the types.  It's ArrayBuffer, not UnsharedArrayBuffer, and "Default" seems to be closer to meaning "the ArrayBuffer that isn't special".

> 
> > Source/JavaScriptCore/runtime/AtomicsObject.cpp:229
> > +    case 3:
> 
> case 3 should return false, and case 4 should return true (along with 1 and
> 2).

Oops.

> 
> > Source/JavaScriptCore/runtime/AtomicsObject.cpp:305
> > +    double timeoutInNanoseconds = timeoutInMilliseconds * 1000;
> 
> Missing 1000?

Oops.

> 
> > Source/JavaScriptCore/runtime/AtomicsObject.cpp:315
> > +    if (timeoutInNanoseconds == timeoutInNanoseconds)
> 
> std::isnan

== is a fundamental operator in C++ that already answers this question.  I don't think we should mandate using helpers for operations that are already primitive.

> 
> > Source/JavaScriptCore/runtime/AtomicsObject.cpp:316
> > +        timeoutInNanoseconds = std::max(0., timeoutInNanoseconds);
> 
> This seems to be a spec thing: is -0. a valid thing here?

The spec wants +0 here, but it doesn't matter.

> 
> > Source/WTF/wtf/Atomics.h:-96
> > -    ASSERT(bitwise_cast<std::atomic<T>*>(location)->is_lock_free() && "expected lock-free type");
> 
> I think you want to keep these asserts.

I removed them because I didn't want this noise everywhere.  If we did want an assertion about the lock freedom of primitive types, we should probably put them somewhere else.

[Filip Pizlo]

Created attachment 293459 [details]
the patch

[WebKit Commit Bot]

Attachment 293459 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:101:  Declaration has space between type name and * in const JSWorkerGlobalScopeBase *thisObject  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:185:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 6 in 105 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 293460 [details]
the patch

[WebKit Commit Bot]

Attachment 293460 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:101:  Declaration has space between type name and * in const JSWorkerGlobalScopeBase *thisObject  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:185:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 6 in 106 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 293460 [details]
the patch

Attachment 293460 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2414059

New failing tests:
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 293473 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Filip Pizlo]

Created attachment 293478 [details]
the patch

[WebKit Commit Bot]

Attachment 293478 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:101:  Declaration has space between type name and * in const JSWorkerGlobalScopeBase *thisObject  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:185:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 6 in 108 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Comment on attachment 293478 [details]
the patch

r=me. Assuming the bots are happy and you intend on importing more tests since this current testing seems a little light.

[Filip Pizlo]

This patch passes everything for me locally.  I think that EWS is seeing unrelated crashes.  But I'll wait a bit before landing.

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/208209

[Simon Fraser (smfr)]

This patch broke bindings tests:
https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK1%20%28Tests%29/builds/1208

Fixing.