New issue
Advanced search Search tips

Issue 889542 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 11
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature

Blocked on:
issue 911800
issue 912480



Sign in to add a comment

Implement a reliable blink::ExecutionContext-to-v8::Isolate conversion

Project Member Reported by hinoka@chromium.org, Sep 26

Issue description

=== From yukishiino@ ===
The mapping from blink::ExecutionContext to v8::Isolate is N:1 mapping, and the mappings are static (not dynamically changing).  So, it's a good idea to have ExecutionContext remember its associated v8::Isolate.  We only have a historical reason about why we're not currently doing so.  The fundamental idea is demonstrated as below:

    class ExecutionContext {
      ExecutionContext(v8::Isolate* isolate)
          : isolate_(isolate) {}

      v8::Isolate* GetV8Isolate() const { return isolate_; }

      v8::Isolate* const isolate_ = nullptr;
    };

Currently we're using ToIsolate(ExecutionContext*) to convert an ExecutionContext to a v8::Isolate, however, this implementation depends on several assumptions and we're not sure whether this function is always working as expected (I think the answer is No).  The way illustrated above is much simpler and always reliable.
===
 

Comment 1 Deleted

I would like to fix this issue if no one working on this.

Status: Started (was: Assigned)
hinoka@ is already working on this issue.  Probably you can find another interesting issue.  :)

Project Member

Comment 4 by bugdroid1@chromium.org, Nov 29

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

commit c66e168679b374e03951e3e312266e24e7f68ae9
Author: Ryan Tseng <hinoka@google.com>
Date: Thu Nov 29 02:33:39 2018

Use static mapping for Blink::ExecutionContext -> V8::Isolate

Bug:  889542 
Change-Id: I59055c2ce9b6af9026ee8284dd3f943cc0d7d179
Reviewed-on: https://chromium-review.googlesource.com/c/1252625
Commit-Queue: Ryan Tseng <hinoka@chromium.org>
Reviewed-by: Hayato Ito <hayato@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611992}
[modify] https://crrev.com/c66e168679b374e03951e3e312266e24e7f68ae9/third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.cc
[modify] https://crrev.com/c66e168679b374e03951e3e312266e24e7f68ae9/third_party/blink/renderer/core/dom/document.cc
[modify] https://crrev.com/c66e168679b374e03951e3e312266e24e7f68ae9/third_party/blink/renderer/core/execution_context/execution_context.cc
[modify] https://crrev.com/c66e168679b374e03951e3e312266e24e7f68ae9/third_party/blink/renderer/core/execution_context/execution_context.h
[modify] https://crrev.com/c66e168679b374e03951e3e312266e24e7f68ae9/third_party/blink/renderer/core/testing/null_execution_context.cc
[modify] https://crrev.com/c66e168679b374e03951e3e312266e24e7f68ae9/third_party/blink/renderer/core/workers/worker_or_worklet_global_scope.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Nov 29

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

commit 5c13e30af46bc3196465353fbf76ab001b567024
Author: Ryan Tseng <hinoka@google.com>
Date: Thu Nov 29 20:32:51 2018

fix blink::ToIsolate after r611992

https://chromium-review.googlesource.com/c/chromium/src/+/1252625
was supposed to do a DCHECK to ensure the semantics of ToIsolate()
remained unchanged, but the check was incorrect.  This fixes the semantics.

The next step is to remove ToIsolate(ExecutionContext).

Bug:  889542 
Change-Id: Ic0a84fa74cafef47e43858f4dae4880b2f7757ec
Reviewed-on: https://chromium-review.googlesource.com/c/1355190
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Ryan Tseng <hinoka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612341}
[modify] https://crrev.com/5c13e30af46bc3196465353fbf76ab001b567024/third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Dec 6

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

commit e206e5c21ee57618fc3c7fa9d59a60360d39f711
Author: Ryan Tseng <hinoka@google.com>
Date: Thu Dec 06 00:39:35 2018

Replace blink::ToIsolate(ExecutionContext) with blink::ExecutionContext->GetIsolate()

And remove blink::ToIsolate(ExecutionContext) altogether.

Bug:  889542 
Change-Id: I33e787acbde47b76cceb77288c82356bc5fa8411
Reviewed-on: https://chromium-review.googlesource.com/c/1355944
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Ryan Tseng <hinoka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614197}
[modify] https://crrev.com/e206e5c21ee57618fc3c7fa9d59a60360d39f711/third_party/blink/renderer/bindings/core/v8/script_event_listener.cc
[modify] https://crrev.com/e206e5c21ee57618fc3c7fa9d59a60360d39f711/third_party/blink/renderer/bindings/core/v8/script_promise_property_base.cc
[modify] https://crrev.com/e206e5c21ee57618fc3c7fa9d59a60360d39f711/third_party/blink/renderer/bindings/core/v8/script_promise_property_test.cc
[modify] https://crrev.com/e206e5c21ee57618fc3c7fa9d59a60360d39f711/third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.cc
[modify] https://crrev.com/e206e5c21ee57618fc3c7fa9d59a60360d39f711/third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.h
[modify] https://crrev.com/e206e5c21ee57618fc3c7fa9d59a60360d39f711/third_party/blink/renderer/core/frame/csp/content_security_policy.cc
[modify] https://crrev.com/e206e5c21ee57618fc3c7fa9d59a60360d39f711/third_party/blink/renderer/core/html/html_frame_element_base.cc
[modify] https://crrev.com/e206e5c21ee57618fc3c7fa9d59a60360d39f711/third_party/blink/renderer/core/messaging/message_port.cc
[modify] https://crrev.com/e206e5c21ee57618fc3c7fa9d59a60360d39f711/third_party/blink/renderer/core/probe/core_probes.cc
[modify] https://crrev.com/e206e5c21ee57618fc3c7fa9d59a60360d39f711/third_party/blink/renderer/core/trustedtypes/trusted_types_util.cc
[modify] https://crrev.com/e206e5c21ee57618fc3c7fa9d59a60360d39f711/third_party/blink/renderer/core/workers/dedicated_worker.cc
[modify] https://crrev.com/e206e5c21ee57618fc3c7fa9d59a60360d39f711/third_party/blink/renderer/core/workers/dedicated_worker_messaging_proxy.cc
[modify] https://crrev.com/e206e5c21ee57618fc3c7fa9d59a60360d39f711/third_party/blink/renderer/core/workers/experimental/task.cc
[modify] https://crrev.com/e206e5c21ee57618fc3c7fa9d59a60360d39f711/third_party/blink/renderer/core/xmlhttprequest/xml_http_request.cc
[modify] https://crrev.com/e206e5c21ee57618fc3c7fa9d59a60360d39f711/third_party/blink/renderer/modules/indexeddb/idb_key_range.cc
[modify] https://crrev.com/e206e5c21ee57618fc3c7fa9d59a60360d39f711/third_party/blink/renderer/modules/payments/abort_payment_respond_with_observer.cc
[modify] https://crrev.com/e206e5c21ee57618fc3c7fa9d59a60360d39f711/third_party/blink/renderer/modules/payments/can_make_payment_respond_with_observer.cc
[modify] https://crrev.com/e206e5c21ee57618fc3c7fa9d59a60360d39f711/third_party/blink/renderer/modules/payments/payment_request_respond_with_observer.cc
[modify] https://crrev.com/e206e5c21ee57618fc3c7fa9d59a60360d39f711/third_party/blink/renderer/modules/service_worker/fetch_respond_with_observer.cc
[modify] https://crrev.com/e206e5c21ee57618fc3c7fa9d59a60360d39f711/third_party/blink/renderer/modules/time_zone_monitor/time_zone_monitor_client.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Dec 6

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

commit e908ae1d722150daddb6a3074842d564e29b6694
Author: Ryan Tseng <hinoka@chromium.org>
Date: Thu Dec 06 18:13:34 2018

Revert "Replace blink::ToIsolate(ExecutionContext) with blink::ExecutionContext->GetIsolate()"

This reverts commit e206e5c21ee57618fc3c7fa9d59a60360d39f711.

Reason for revert: Crashing
Crash report: crbug.com/912480
Fuzzer report:  crbug.com/912451 

Likely due to nullptr contexts passed in blink::probe::AsyncTaskCanceled() and blink::probe::AsyncTaskCanceledBreakable()

Original change's description:
> Replace blink::ToIsolate(ExecutionContext) with blink::ExecutionContext->GetIsolate()
> 
> And remove blink::ToIsolate(ExecutionContext) altogether.
> 
> Bug:  889542 
> Change-Id: I33e787acbde47b76cceb77288c82356bc5fa8411
> Reviewed-on: https://chromium-review.googlesource.com/c/1355944
> Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Commit-Queue: Ryan Tseng <hinoka@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#614197}

TBR=hinoka@chromium.org,yukishiino@chromium.org,haraken@chromium.org

Change-Id: Icea742f3e380e87491d1a8606cf7c10e5c2a7c4c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  889542 
Reviewed-on: https://chromium-review.googlesource.com/c/1365881
Reviewed-by: Ryan Tseng <hinoka@chromium.org>
Commit-Queue: Ryan Tseng <hinoka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614419}
[modify] https://crrev.com/e908ae1d722150daddb6a3074842d564e29b6694/third_party/blink/renderer/bindings/core/v8/script_event_listener.cc
[modify] https://crrev.com/e908ae1d722150daddb6a3074842d564e29b6694/third_party/blink/renderer/bindings/core/v8/script_promise_property_base.cc
[modify] https://crrev.com/e908ae1d722150daddb6a3074842d564e29b6694/third_party/blink/renderer/bindings/core/v8/script_promise_property_test.cc
[modify] https://crrev.com/e908ae1d722150daddb6a3074842d564e29b6694/third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.cc
[modify] https://crrev.com/e908ae1d722150daddb6a3074842d564e29b6694/third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.h
[modify] https://crrev.com/e908ae1d722150daddb6a3074842d564e29b6694/third_party/blink/renderer/core/frame/csp/content_security_policy.cc
[modify] https://crrev.com/e908ae1d722150daddb6a3074842d564e29b6694/third_party/blink/renderer/core/html/html_frame_element_base.cc
[modify] https://crrev.com/e908ae1d722150daddb6a3074842d564e29b6694/third_party/blink/renderer/core/messaging/message_port.cc
[modify] https://crrev.com/e908ae1d722150daddb6a3074842d564e29b6694/third_party/blink/renderer/core/probe/core_probes.cc
[modify] https://crrev.com/e908ae1d722150daddb6a3074842d564e29b6694/third_party/blink/renderer/core/trustedtypes/trusted_types_util.cc
[modify] https://crrev.com/e908ae1d722150daddb6a3074842d564e29b6694/third_party/blink/renderer/core/workers/dedicated_worker.cc
[modify] https://crrev.com/e908ae1d722150daddb6a3074842d564e29b6694/third_party/blink/renderer/core/workers/dedicated_worker_messaging_proxy.cc
[modify] https://crrev.com/e908ae1d722150daddb6a3074842d564e29b6694/third_party/blink/renderer/core/workers/experimental/task.cc
[modify] https://crrev.com/e908ae1d722150daddb6a3074842d564e29b6694/third_party/blink/renderer/core/xmlhttprequest/xml_http_request.cc
[modify] https://crrev.com/e908ae1d722150daddb6a3074842d564e29b6694/third_party/blink/renderer/modules/indexeddb/idb_key_range.cc
[modify] https://crrev.com/e908ae1d722150daddb6a3074842d564e29b6694/third_party/blink/renderer/modules/payments/abort_payment_respond_with_observer.cc
[modify] https://crrev.com/e908ae1d722150daddb6a3074842d564e29b6694/third_party/blink/renderer/modules/payments/can_make_payment_respond_with_observer.cc
[modify] https://crrev.com/e908ae1d722150daddb6a3074842d564e29b6694/third_party/blink/renderer/modules/payments/payment_request_respond_with_observer.cc
[modify] https://crrev.com/e908ae1d722150daddb6a3074842d564e29b6694/third_party/blink/renderer/modules/service_worker/fetch_respond_with_observer.cc
[modify] https://crrev.com/e908ae1d722150daddb6a3074842d564e29b6694/third_party/blink/renderer/modules/time_zone_monitor/time_zone_monitor_client.cc

Blockedon: 912480
Blockedon: 911800
Project Member

Comment 10 by bugdroid1@chromium.org, Dec 11

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

commit 32929f531a58a3cadf185d351d1a3382863fa062
Author: Ryan Tseng <hinoka@google.com>
Date: Tue Dec 11 06:11:19 2018

Reland: Replace blink::ToIsolate(ExecutionContext) with blink::ExecutionContext->GetIsolate()

And remove blink::ToIsolate(ExecutionContext) altogether.

Originally https://chromium-review.googlesource.com/c/chromium/src/+/1355944
The difference is added checks in:
* AsyncTaskCanceled
* AllAsyncTasksCanceled
for nullptr ExecutionContext.

Bug:  889542 ,  911800 
Change-Id: Icef90fadbc89019118119f044dde8f10ddffa72f
Reviewed-on: https://chromium-review.googlesource.com/c/1366358
Commit-Queue: Ryan Tseng <hinoka@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615445}
[modify] https://crrev.com/32929f531a58a3cadf185d351d1a3382863fa062/third_party/blink/renderer/bindings/core/v8/script_event_listener.cc
[modify] https://crrev.com/32929f531a58a3cadf185d351d1a3382863fa062/third_party/blink/renderer/bindings/core/v8/script_promise_property_base.cc
[modify] https://crrev.com/32929f531a58a3cadf185d351d1a3382863fa062/third_party/blink/renderer/bindings/core/v8/script_promise_property_test.cc
[modify] https://crrev.com/32929f531a58a3cadf185d351d1a3382863fa062/third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.cc
[modify] https://crrev.com/32929f531a58a3cadf185d351d1a3382863fa062/third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.h
[modify] https://crrev.com/32929f531a58a3cadf185d351d1a3382863fa062/third_party/blink/renderer/core/frame/csp/content_security_policy.cc
[modify] https://crrev.com/32929f531a58a3cadf185d351d1a3382863fa062/third_party/blink/renderer/core/html/html_frame_element_base.cc
[modify] https://crrev.com/32929f531a58a3cadf185d351d1a3382863fa062/third_party/blink/renderer/core/messaging/message_port.cc
[modify] https://crrev.com/32929f531a58a3cadf185d351d1a3382863fa062/third_party/blink/renderer/core/probe/core_probes.cc
[modify] https://crrev.com/32929f531a58a3cadf185d351d1a3382863fa062/third_party/blink/renderer/core/trustedtypes/trusted_types_util.cc
[modify] https://crrev.com/32929f531a58a3cadf185d351d1a3382863fa062/third_party/blink/renderer/core/workers/dedicated_worker.cc
[modify] https://crrev.com/32929f531a58a3cadf185d351d1a3382863fa062/third_party/blink/renderer/core/workers/dedicated_worker_messaging_proxy.cc
[modify] https://crrev.com/32929f531a58a3cadf185d351d1a3382863fa062/third_party/blink/renderer/core/workers/experimental/task.cc
[modify] https://crrev.com/32929f531a58a3cadf185d351d1a3382863fa062/third_party/blink/renderer/core/xmlhttprequest/xml_http_request.cc
[modify] https://crrev.com/32929f531a58a3cadf185d351d1a3382863fa062/third_party/blink/renderer/modules/indexeddb/idb_key_range.cc
[modify] https://crrev.com/32929f531a58a3cadf185d351d1a3382863fa062/third_party/blink/renderer/modules/payments/abort_payment_respond_with_observer.cc
[modify] https://crrev.com/32929f531a58a3cadf185d351d1a3382863fa062/third_party/blink/renderer/modules/payments/can_make_payment_respond_with_observer.cc
[modify] https://crrev.com/32929f531a58a3cadf185d351d1a3382863fa062/third_party/blink/renderer/modules/payments/payment_request_respond_with_observer.cc
[modify] https://crrev.com/32929f531a58a3cadf185d351d1a3382863fa062/third_party/blink/renderer/modules/service_worker/fetch_respond_with_observer.cc
[modify] https://crrev.com/32929f531a58a3cadf185d351d1a3382863fa062/third_party/blink/renderer/modules/time_zone_monitor/time_zone_monitor_client.cc

Status: Fixed (was: Started)

Sign in to add a comment