New issue
Advanced search Search tips

Issue 669812 link

Starred by 2 users
Status: Fixed
Owner:
haraken@chromium.org
Closed: Dec 2016
Cc:
yukishiino@chromium.org
lfg@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Remove RefPtr<ScriptState> stored in DOM objects

Project Member Reported by haraken@chromium.org, Nov 30 2016 
[ConstructorCallWith=ScriptState] is sometimes used to cache RefPtr<ScriptState> on a DOM object when the DOM object is constructed. Then the ScriptState is used when subsequent DOM attributes/methods are called. This is a wrong programming pattern because it will leak the ScriptState to the world that called the DOM attributes/methods. We should use [CallWith=ScriptState] at every DOM attribute/method.

To prevent the mistake, we should remove [ConstructorCallWith=ScriptState].
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 30 2016 

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

commit 346fe565b766ec090e9dd444b03542b02bab732a
Author: haraken <haraken@chromium.org>
Date: Wed Nov 30 11:24:59 2016

Remove [ConstructorCallWith=ScriptState] from Sensor

We're deprecating [ConstructorCallWith=ScriptState] since it can lead to
a programming error that leaks ScriptStates to other worlds (see the bug).
This CL replaces it with [ConstructorCallWith=ExecutionContext].

BUG= 669812 

Review-Url: https://codereview.chromium.org/2537863003
Cr-Commit-Position: refs/heads/master@{#435217}

[modify] https://crrev.com/346fe565b766ec090e9dd444b03542b02bab732a/third_party/WebKit/Source/modules/sensor/Accelerometer.cpp
[modify] https://crrev.com/346fe565b766ec090e9dd444b03542b02bab732a/third_party/WebKit/Source/modules/sensor/Accelerometer.h
[modify] https://crrev.com/346fe565b766ec090e9dd444b03542b02bab732a/third_party/WebKit/Source/modules/sensor/Accelerometer.idl
[modify] https://crrev.com/346fe565b766ec090e9dd444b03542b02bab732a/third_party/WebKit/Source/modules/sensor/AmbientLightSensor.cpp
[modify] https://crrev.com/346fe565b766ec090e9dd444b03542b02bab732a/third_party/WebKit/Source/modules/sensor/AmbientLightSensor.h
[modify] https://crrev.com/346fe565b766ec090e9dd444b03542b02bab732a/third_party/WebKit/Source/modules/sensor/AmbientLightSensor.idl
[modify] https://crrev.com/346fe565b766ec090e9dd444b03542b02bab732a/third_party/WebKit/Source/modules/sensor/Gyroscope.cpp
[modify] https://crrev.com/346fe565b766ec090e9dd444b03542b02bab732a/third_party/WebKit/Source/modules/sensor/Gyroscope.h
[modify] https://crrev.com/346fe565b766ec090e9dd444b03542b02bab732a/third_party/WebKit/Source/modules/sensor/Gyroscope.idl
[modify] https://crrev.com/346fe565b766ec090e9dd444b03542b02bab732a/third_party/WebKit/Source/modules/sensor/Magnetometer.cpp
[modify] https://crrev.com/346fe565b766ec090e9dd444b03542b02bab732a/third_party/WebKit/Source/modules/sensor/Magnetometer.h
[modify] https://crrev.com/346fe565b766ec090e9dd444b03542b02bab732a/third_party/WebKit/Source/modules/sensor/Magnetometer.idl
[modify] https://crrev.com/346fe565b766ec090e9dd444b03542b02bab732a/third_party/WebKit/Source/modules/sensor/Sensor.cpp
[modify] https://crrev.com/346fe565b766ec090e9dd444b03542b02bab732a/third_party/WebKit/Source/modules/sensor/Sensor.h
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 30 2016 

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

commit 989e930a3f48255272eeca48144c0e744fb47878
Author: haraken <haraken@chromium.org>
Date: Wed Nov 30 13:48:39 2016

Remove [ConstructorCallWith=ScriptState] from ShapeDetector

We want to deprecate [ConstructorCallWith=ScriptState] since it has lead to
a programming error that leaks JS objects between worlds (see the bug).
This CL replaces [ConstructorCallWith=ScriptState] with [ConstructorCallWith=Document].

BUG= 669812 

Review-Url: https://codereview.chromium.org/2539073002
Cr-Commit-Position: refs/heads/master@{#435243}

[modify] https://crrev.com/989e930a3f48255272eeca48144c0e744fb47878/third_party/WebKit/Source/modules/shapedetection/BarcodeDetector.cpp
[modify] https://crrev.com/989e930a3f48255272eeca48144c0e744fb47878/third_party/WebKit/Source/modules/shapedetection/BarcodeDetector.h
[modify] https://crrev.com/989e930a3f48255272eeca48144c0e744fb47878/third_party/WebKit/Source/modules/shapedetection/BarcodeDetector.idl
[modify] https://crrev.com/989e930a3f48255272eeca48144c0e744fb47878/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp
[modify] https://crrev.com/989e930a3f48255272eeca48144c0e744fb47878/third_party/WebKit/Source/modules/shapedetection/FaceDetector.h
[modify] https://crrev.com/989e930a3f48255272eeca48144c0e744fb47878/third_party/WebKit/Source/modules/shapedetection/FaceDetector.idl

Comment 3 by haraken@chromium.org, Dec 2 2016 

Unfortunately I noticed that we cannot deprecate [ConstructorCallWith=ScriptState] entirely. Constructors of some DOM objects require ScriptState* to execute V8 things:

- core/xmlhttprequest/XMLHttpRequest.idl
- core/events/KeyboardEvent.idl
- core/events/MouseEvent.idl
- modules/serviceworkers/ForeignFetchEvent.idl
- modules/serviceworkers/FetchEvent.idl
- modules/fetch/Request.idl
- modules/fetch/Response.idl

[ConstructorCallWith=ScriptState] itself is not a problem. The real problem is that it is sometimes to be used to cache ScriptState on the DOM object; i.e., the problem is RefPtr<ScriptState> on the DOM object.

I'll give up deprecating [ConstructorCallWith=ScriptState] and instead try to remove  RefPtr<ScriptState> on DOM objects.

Comment 4 by bashi@chromium.org, Dec 2 2016 

Does it make sense to add a note which describes the problem in https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Source/bindings/IDLExtendedAttributes.md ?

Comment 5 by haraken@chromium.org, Dec 2 2016

Summary: Remove RefPtr<ScriptState> stored in DOM objects (was: Remove [ConstructorCallWith=ScriptState])
Project Member

Comment 6 by bugdroid1@chromium.org, Dec 2 2016 

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

commit 0db356f8c794ed884c3e13949973e148a0d54afb
Author: haraken <haraken@chromium.org>
Date: Fri Dec 02 06:39:16 2016

Remove [ConstructorCallWith=ScriptState] from Payments

We want to deprecate [ConstructorCallWith=ScriptState] since it has lead to
a programming error that leaks JS objects between worlds (see the bug).
This CL replaces [ConstructorCallWith=ScriptState] with [ConstructorCallWith=Document].

BUG= 669812 

Review-Url: https://codereview.chromium.org/2541663003
Cr-Commit-Position: refs/heads/master@{#435869}

[modify] https://crrev.com/0db356f8c794ed884c3e13949973e148a0d54afb/third_party/WebKit/Source/modules/payments/AbortTest.cpp
[modify] https://crrev.com/0db356f8c794ed884c3e13949973e148a0d54afb/third_party/WebKit/Source/modules/payments/ActivePaymentTest.cpp
[modify] https://crrev.com/0db356f8c794ed884c3e13949973e148a0d54afb/third_party/WebKit/Source/modules/payments/CompleteTest.cpp
[modify] https://crrev.com/0db356f8c794ed884c3e13949973e148a0d54afb/third_party/WebKit/Source/modules/payments/OnPaymentResponseTest.cpp
[modify] https://crrev.com/0db356f8c794ed884c3e13949973e148a0d54afb/third_party/WebKit/Source/modules/payments/PaymentAppManager.cpp
[modify] https://crrev.com/0db356f8c794ed884c3e13949973e148a0d54afb/third_party/WebKit/Source/modules/payments/PaymentAppManager.h
[modify] https://crrev.com/0db356f8c794ed884c3e13949973e148a0d54afb/third_party/WebKit/Source/modules/payments/PaymentAppManager.idl
[modify] https://crrev.com/0db356f8c794ed884c3e13949973e148a0d54afb/third_party/WebKit/Source/modules/payments/PaymentAppServiceWorkerRegistration.cpp
[modify] https://crrev.com/0db356f8c794ed884c3e13949973e148a0d54afb/third_party/WebKit/Source/modules/payments/PaymentRequest.cpp
[modify] https://crrev.com/0db356f8c794ed884c3e13949973e148a0d54afb/third_party/WebKit/Source/modules/payments/PaymentRequest.h
[modify] https://crrev.com/0db356f8c794ed884c3e13949973e148a0d54afb/third_party/WebKit/Source/modules/payments/PaymentRequest.idl
[modify] https://crrev.com/0db356f8c794ed884c3e13949973e148a0d54afb/third_party/WebKit/Source/modules/payments/PaymentRequestDetailsTest.cpp
[modify] https://crrev.com/0db356f8c794ed884c3e13949973e148a0d54afb/third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp
[modify] https://crrev.com/0db356f8c794ed884c3e13949973e148a0d54afb/third_party/WebKit/Source/modules/payments/PaymentRequestUpdateEventTest.cpp
Project Member

Comment 7 by bugdroid1@chromium.org, Dec 2 2016 

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

commit efbd06c5569a5a7e6e2dda507e3402bf7eec5675
Author: haraken <haraken@chromium.org>
Date: Fri Dec 02 08:04:38 2016

Update documentation for [CallWith] and [ConstructorCallWith]

BUG= 669812 

Review-Url: https://codereview.chromium.org/2544253002
Cr-Commit-Position: refs/heads/master@{#435879}

[modify] https://crrev.com/efbd06c5569a5a7e6e2dda507e3402bf7eec5675/third_party/WebKit/Source/bindings/IDLExtendedAttributes.md
Project Member

Comment 8 by bugdroid1@chromium.org, Dec 2 2016 

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

commit 05528ffaebde3c6b3534001096dcbdcc4814a931
Author: haraken <haraken@chromium.org>
Date: Fri Dec 02 08:19:00 2016

Remove RefPtr<ScriptState> from PromiseRejectionEvent

RefPtr<ScriptState> stored in a DOM object has a risk of leaking ScriptStates
to another world (see the bug for more details). This CL replaces
PromiseRejectionEvent::m_scriptState with PromiseRejectionEvent::m_world.
This matches what we're doing for ErrorEvent.

BUG= 669812 

Review-Url: https://codereview.chromium.org/2544883005
Cr-Commit-Position: refs/heads/master@{#435882}

[modify] https://crrev.com/05528ffaebde3c6b3534001096dcbdcc4814a931/third_party/WebKit/Source/core/events/PromiseRejectionEvent.cpp
[modify] https://crrev.com/05528ffaebde3c6b3534001096dcbdcc4814a931/third_party/WebKit/Source/core/events/PromiseRejectionEvent.h
Project Member

Comment 9 by bugdroid1@chromium.org, Dec 2 2016 

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

commit 9784a00e2cd70f8828ead651103b5ab22e3c6ac4
Author: haraken <haraken@chromium.org>
Date: Fri Dec 02 08:21:36 2016

Remove RefPtr<ScriptState> from FetchEvent

RefPtr<ScriptState> stored in DOM objects has a risk of leaking ScriptStates
to unexpected worlds. We should explicitly pass in ScriptState when we need it.

BUG= 669812 

Review-Url: https://codereview.chromium.org/2539183006
Cr-Commit-Position: refs/heads/master@{#435884}

[modify] https://crrev.com/9784a00e2cd70f8828ead651103b5ab22e3c6ac4/third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp
[modify] https://crrev.com/9784a00e2cd70f8828ead651103b5ab22e3c6ac4/third_party/WebKit/Source/modules/serviceworkers/FetchEvent.h
[modify] https://crrev.com/9784a00e2cd70f8828ead651103b5ab22e3c6ac4/third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp
Project Member

Comment 10 by bugdroid1@chromium.org, Dec 2 2016 

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

commit 70bae06b6a59e2ce25eb9f1c0b02d368e7a684eb
Author: haraken <haraken@chromium.org>
Date: Fri Dec 02 09:19:35 2016

Remove [ConstructorCallWith=ScriptState] from Internals.idl

It's unused.

BUG= 669812 

Review-Url: https://codereview.chromium.org/2547803002
Cr-Commit-Position: refs/heads/master@{#435893}

[modify] https://crrev.com/70bae06b6a59e2ce25eb9f1c0b02d368e7a684eb/third_party/WebKit/Source/core/testing/Internals.idl
Project Member

Comment 12 by bugdroid1@chromium.org, Dec 7 2016 

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

commit a1229b7ad6ec062281bcd389dcc1ca93b49f5d2f
Author: haraken <haraken@chromium.org>
Date: Wed Dec 07 03:42:46 2016

Remove RefPtr<ScriptState> from MediaSession

RefPtr<ScriptState> stored in DOM objects has a risk of leaking ScriptStates
to unexpected worlds. We should explicitly pass in ScriptState when we need it.

This CL makes MediaSession a ContextLifecycleObserver.

BUG= 669812 

Review-Url: https://codereview.chromium.org/2545893003
Cr-Commit-Position: refs/heads/master@{#436853}

[modify] https://crrev.com/a1229b7ad6ec062281bcd389dcc1ca93b49f5d2f/third_party/WebKit/Source/modules/mediasession/MediaSession.cpp
[modify] https://crrev.com/a1229b7ad6ec062281bcd389dcc1ca93b49f5d2f/third_party/WebKit/Source/modules/mediasession/MediaSession.h
[modify] https://crrev.com/a1229b7ad6ec062281bcd389dcc1ca93b49f5d2f/third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.cpp

Comment 13 by haraken@chromium.org, Dec 7 2016

Status: Fixed (was: Assigned)
I finished fixing all problematic RefPtr<ScriptState>s.
►

Sign in to add a comment