New issue
Advanced search Search tips

Issue 897046 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 5
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocking:
issue 894838



Sign in to add a comment

Export original values of MessagePort and MessageChannel to V8 Extras

Project Member Reported by ricea@chromium.org, Oct 19

Issue description

The transferable streams implementation needs to call many methods and accessors on MessagePort and MessageChannel. Since for security and robustness reasons we cannot use versions that have been tampered with by the page, we have to capture these values before any user JavaScript runs.
 
I have attached the script I will use to determine if adding the extra items to the bindings object has a measurable performance impact. It just creates an iframe 10000 times, running JavaScript in each one to make sure the JavaScript context is initialised.

Timings prior to changing anything (release build, no DCHECKs, no components):

iframe creation: 62370.404052734375ms
iframe creation: 63867.81689453125ms
iframe creation: 63377.1171875ms

It's possible there's some wasted work being done in this script since 6ms seems like a long time to create an iframe.
iframe-timer.html
656 bytes View Download
Cc: yhirano@chromium.org yukishiino@chromium.org
Timings after adding the initialisation code:

iframe creation: 63696.813720703125ms
iframe creation: 63577.3271484375ms
iframe creation: 64395.624267578125ms

It's within the margin of error so I think it is okay.
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 26

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/821303d2ee44304dc272587ce1513ae0a9e30d0d

commit 821303d2ee44304dc272587ce1513ae0a9e30d0d
Author: Adam Rice <ricea@chromium.org>
Date: Fri Oct 26 10:52:16 2018

Streams: Get MessageChannel-related values from bindings

Copy the the original values of selected DOM constructors, methods and
getters to the binding object during global object initialisation. Then
access them via the binding object in the V8 extras transferable streams
implementation. This protects the implementation from modifications to
the global object.

Constructors copied:
MessageChannel
DOMException

Methods copied:
EventTarget.prototype.addEventListener
MessagePort.prototype.postMessage
MessagePort.prototype.close
MessagePort.prototype.start

Accessors copied:
MessageChannel.prototype.port1
MessageChannel.prototype.port2
MessageEvent.prototype.data
DOMException.prototype.message
DOMException.prototype.name

BUG= 897046 

Change-Id: I72e40951e5c45f51c957dfa277044a210b054675
Reviewed-on: https://chromium-review.googlesource.com/c/1291191
Commit-Queue: Adam Rice <ricea@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603053}
[modify] https://crrev.com/821303d2ee44304dc272587ce1513ae0a9e30d0d/third_party/blink/renderer/bindings/core/v8/initialize_v8_extras_binding.cc
[modify] https://crrev.com/821303d2ee44304dc272587ce1513ae0a9e30d0d/third_party/blink/renderer/bindings/core/v8/initialize_v8_extras_binding.h
[modify] https://crrev.com/821303d2ee44304dc272587ce1513ae0a9e30d0d/third_party/blink/renderer/bindings/core/v8/local_window_proxy.cc
[modify] https://crrev.com/821303d2ee44304dc272587ce1513ae0a9e30d0d/third_party/blink/renderer/bindings/core/v8/worker_or_worklet_script_controller.cc
[modify] https://crrev.com/821303d2ee44304dc272587ce1513ae0a9e30d0d/third_party/blink/renderer/core/streams/CommonOperations.js
[modify] https://crrev.com/821303d2ee44304dc272587ce1513ae0a9e30d0d/third_party/blink/renderer/core/streams/ReadableStream.js

Status: Fixed (was: Assigned)
Status: Assigned (was: Fixed)
Reopening due to a performance regression.
Project Member

Comment 6 by bugdroid1@chromium.org, Oct 31

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2c36f81b038bdd7ce75bdf73c626170d7ed3b871

commit 2c36f81b038bdd7ce75bdf73c626170d7ed3b871
Author: Adam Rice <ricea@chromium.org>
Date: Wed Oct 31 07:21:20 2018

Streams: Put binding originals behind a flag

Copying MessageChannel and other functions to the binding object caused
a memory regression. As a temporary workaround, only copy them to the
binding object when the TransferableStreams feature is enabled.

BUG= 897046 ,900141,899637

Change-Id: If312f2e0967a19e9db427f60125d96c48d45fdc7
Reviewed-on: https://chromium-review.googlesource.com/c/1307534
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604178}
[modify] https://crrev.com/2c36f81b038bdd7ce75bdf73c626170d7ed3b871/third_party/blink/renderer/bindings/core/v8/initialize_v8_extras_binding.cc
[modify] https://crrev.com/2c36f81b038bdd7ce75bdf73c626170d7ed3b871/third_party/blink/renderer/core/streams/readable_stream_operations.cc
[modify] https://crrev.com/2c36f81b038bdd7ce75bdf73c626170d7ed3b871/third_party/blink/renderer/core/streams/readable_stream_operations_test.cc
[modify] https://crrev.com/2c36f81b038bdd7ce75bdf73c626170d7ed3b871/third_party/blink/renderer/platform/runtime_enabled_features.json5

Status: Fixed (was: Assigned)

Sign in to add a comment