Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2339683006:
[Blink] Modify SuspendableScriptExecutor to take a v8::Function (Closed)
|
Created:
4 years, 3 months ago by Devlin Modified:
3 years, 4 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, dglazkov+blink, blink-reviews, chromium-apps-reviews_chromium.org, kinuko+watch, blink-reviews-api_chromium.org, nasko, nhiroki, yhirano, dgozman Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Blink] Modify SuspendableScriptExecutor to take a v8::Function
Allow SuspendableScriptExecutor to take a v8::Function so that it can be
used from Extensions code. Also wire up WebLocalFrame and ScriptContext
to have handles to use it. Convert a couple of basic extensions call
sites as a POC.
BUG=629431
Committed: https://crrev.com/a9bc8ccd2becc952e4cea2d8a053247a481dd483
Cr-Commit-Position: refs/heads/master@{#423548}
Patch Set 1 : polish #
Total comments: 21
Patch Set 2 : Daniels #
Total comments: 6
Patch Set 3 : In Prog #
Total comments: 10
Patch Set 4 : Haraken's #
Total comments: 10
Patch Set 5 : mark method as deprecated #
Total comments: 1
Patch Set 6 : Win compile fix #Messages
Total messages: 60 (26 generated)
Description was changed from ========== totaltest BUG= ========== to ========== [Blink] Modify SuspendableScriptExecutor to take a v8::Function Allow SuspendableScriptExecutor to take a v8::Function so that it can be used from Extensions code. Also wire up WebLocalFrame and ScriptContext to have handles to use it. Convert a couple of basic extensions call sites as a POC. BUG=629431 ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
rdevlin.cronin@chromium.org changed reviewers: + dcheng@chromium.org
Heya Daniel, here's a first attempt. Note that I'm not an expert in blink or v8, so I'm pretty much making this up as I go. If something look suspect, it probably is. :) https://codereview.chromium.org/2339683006/diff/60001/extensions/renderer/scr... File extensions/renderer/script_context.cc (right): https://codereview.chromium.org/2339683006/diff/60001/extensions/renderer/scr... extensions/renderer/script_context.cc:216: } else { This can happen in the case of e.g. ServiceWorker contexts. What should we do here? https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/DEPS (right): https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/DEPS:16: "+v8", I'm guessing folks will object to this.
friendly ping. :)
dcheng@chromium.org changed reviewers: + falken@chromium.org, haraken@chromium.org
+haraken for Oilpan question +falken for SW question, and whether service workers can run nested message loops. https://codereview.chromium.org/2339683006/diff/60001/extensions/renderer/scr... File extensions/renderer/script_context.cc (right): https://codereview.chromium.org/2339683006/diff/60001/extensions/renderer/scr... extensions/renderer/script_context.cc:216: } else { On 2016/09/16 18:16:31, Devlin wrote: > This can happen in the case of e.g. ServiceWorker contexts. What should we do > here? Handwaving a bit... maybe this is OK? I don't think service workers can go into nested run loops or do things that would cause script execution to be suspended... so in theory, we shouldn't up doing work in nested message loops that surprises code above it in the stack. +falken to confirm https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/DEPS (right): https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/DEPS:16: "+v8", On 2016/09/16 18:16:31, Devlin (catching up) wrote: > I'm guessing folks will object to this. I think this is OK (I'm a little surprised this is required, it looks like there's a bunch of references to v8 types already) https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:30: HeapVector<ScriptSourceCode> m_sources; I'm a little confused by the ownership model of ScriptSourceCode. It doesn't inherit from GarbageCollected or GarbageCollectedFinalized, but it has Member variables. How does this work? +haraken https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:47: // resumed and contextDestroyed LifecycleObserver methods Hmmm. I'm not sure /where/ this comment belongs, but probably not here (this thing isn't a ContextLifecycleObserver) Maybe we can remove this comment, since it doesn't really make sense... https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:78: Vector<std::unique_ptr<ScopedPersistent<v8::Value>>> m_args; Maybe V8PersistentValueVector from https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... will be useful here? https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:86: m_args.append(std::unique_ptr<ScopedPersistent<v8::Value>>(new ScopedPersistent<v8::Value>(isolate, argv[i]))); wrapUnique from wtf/PtrUtil.h will make this a little easier to write. https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/SuspendableScriptExecutor.h (right): https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/SuspendableScriptExecutor.h:11: #include "v8/include/v8.h" Oh, maybe try cheating and putting this in <>, which is what all the other files do. =)
https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:30: HeapVector<ScriptSourceCode> m_sources; On 2016/09/27 08:30:57, dcheng wrote: > I'm a little confused by the ownership model of ScriptSourceCode. It doesn't > inherit from GarbageCollected or GarbageCollectedFinalized, but it has Member > variables. How does this work? +haraken Yes, because ScriptSourceCode is used only as a part-of object in a GarbageCollected object. The owner GarbageCollected object needs to trace ScriptSourceCode. Then ScriptSourceCode's members are traced.
https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:30: HeapVector<ScriptSourceCode> m_sources; On 2016/09/27 08:39:45, haraken wrote: > On 2016/09/27 08:30:57, dcheng wrote: > > I'm a little confused by the ownership model of ScriptSourceCode. It doesn't > > inherit from GarbageCollected or GarbageCollectedFinalized, but it has Member > > variables. How does this work? +haraken > > Yes, because ScriptSourceCode is used only as a part-of object in a > GarbageCollected object. > > The owner GarbageCollected object needs to trace ScriptSourceCode. Then > ScriptSourceCode's members are traced. I see. How come Oilpan doesn't warn that the holder class here isn't GCed though? It's allocated and stored in a unique_ptr, which should be an error, right?
https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:30: HeapVector<ScriptSourceCode> m_sources; On 2016/09/27 09:02:59, dcheng wrote: > On 2016/09/27 08:39:45, haraken wrote: > > On 2016/09/27 08:30:57, dcheng wrote: > > > I'm a little confused by the ownership model of ScriptSourceCode. It > doesn't > > > inherit from GarbageCollected or GarbageCollectedFinalized, but it has > Member > > > variables. How does this work? +haraken > > > > Yes, because ScriptSourceCode is used only as a part-of object in a > > GarbageCollected object. > > > > The owner GarbageCollected object needs to trace ScriptSourceCode. Then > > ScriptSourceCode's members are traced. > > I see. How come Oilpan doesn't warn that the holder class here isn't GCed > though? It's allocated and stored in a unique_ptr, which should be an error, > right? ScriptSourceCode is in HeapVector (i.e., a GCed object). So this is fine.
https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/DEPS (right): https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/DEPS:16: "+v8", On 2016/09/27 08:30:57, dcheng wrote: > On 2016/09/16 18:16:31, Devlin (catching up) wrote: > > I'm guessing folks will object to this. > > I think this is OK (I'm a little surprised this is required, it looks like > there's a bunch of references to v8 types already) No longer needed with #include<>. https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:47: // resumed and contextDestroyed LifecycleObserver methods On 2016/09/27 08:30:57, dcheng wrote: > Hmmm. I'm not sure /where/ this comment belongs, but probably not here (this > thing isn't a ContextLifecycleObserver) > > Maybe we can remove this comment, since it doesn't really make sense... I'm happy to remove it. Done. https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:78: Vector<std::unique_ptr<ScopedPersistent<v8::Value>>> m_args; On 2016/09/27 08:30:57, dcheng wrote: > Maybe V8PersistentValueVector from > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... > will be useful here? Handy! Done. https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:86: m_args.append(std::unique_ptr<ScopedPersistent<v8::Value>>(new ScopedPersistent<v8::Value>(isolate, argv[i]))); On 2016/09/27 08:30:57, dcheng wrote: > wrapUnique from wtf/PtrUtil.h will make this a little easier to write. Unnecessary with V8PersistentValueVector.
https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:30: HeapVector<ScriptSourceCode> m_sources; On 2016/09/27 13:25:41, haraken wrote: > On 2016/09/27 09:02:59, dcheng wrote: > > On 2016/09/27 08:39:45, haraken wrote: > > > On 2016/09/27 08:30:57, dcheng wrote: > > > > I'm a little confused by the ownership model of ScriptSourceCode. It > > doesn't > > > > inherit from GarbageCollected or GarbageCollectedFinalized, but it has > > Member > > > > variables. How does this work? +haraken > > > > > > Yes, because ScriptSourceCode is used only as a part-of object in a > > > GarbageCollected object. > > > > > > The owner GarbageCollected object needs to trace ScriptSourceCode. Then > > > ScriptSourceCode's members are traced. > > > > I see. How come Oilpan doesn't warn that the holder class here isn't GCed > > though? It's allocated and stored in a unique_ptr, which should be an error, > > right? > > ScriptSourceCode is in HeapVector (i.e., a GCed object). So this is fine. So just to confirm, we have the following ownership: SuspendableScriptExecutor (GarbageCollectedFinalized) std::unique_ptr<SuspendableScriptExecutor::Executor> (Executor is not GCed) HeapVector<ScriptSourceCode> (ScriptSourceCode is not GCed) Member<ScriptResource>, Member<ScriptStreamer>, etc... (GarbageCollectedFinalized) Though both SuspendableScriptExecutor::Executor and ScriptSourceCode are both not GCed, they define trace() methods, so this is OK, because Oilpan can still reach the garbage collected members stored inside them. Is this correct?
https://codereview.chromium.org/2339683006/diff/60001/extensions/renderer/scr... File extensions/renderer/script_context.cc (right): https://codereview.chromium.org/2339683006/diff/60001/extensions/renderer/scr... extensions/renderer/script_context.cc:216: } else { On 2016/09/27 08:30:57, dcheng wrote: > On 2016/09/16 18:16:31, Devlin wrote: > > This can happen in the case of e.g. ServiceWorker contexts. What should we do > > here? > > Handwaving a bit... maybe this is OK? I don't think service workers can go into > nested run loops or do things that would cause script execution to be > suspended... so in theory, we shouldn't up doing work in nested message loops > that surprises code above it in the stack. > > +falken to confirm +nhiroki, +yhirano I probably don't understand enough to give an authoritative answer. dcheng tells me sync APIs are the key to nested run loops. Service worker in principle doesn't have sync APIs except for importScripts(). During the importScripts call we wouldn't be dispatching an event to the SW, since the browser process waits for the SW to finish starting up before it sends messages to the renderer to dispatch events. Also currently FileReaderSync is available but that is a bug <https://github.com/w3c/ServiceWorker/issues/735>. If it's relevant, SW also can't terminate itself synchronously: the browser process orders the renderer to terminate the worker. This used to result in a terminateExecution but it changed a bit recently in https://crbug.com/487050 to have a more graceful shutdown.
On 2016/09/27 22:24:25, dcheng wrote: > https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): > > https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:30: > HeapVector<ScriptSourceCode> m_sources; > On 2016/09/27 13:25:41, haraken wrote: > > On 2016/09/27 09:02:59, dcheng wrote: > > > On 2016/09/27 08:39:45, haraken wrote: > > > > On 2016/09/27 08:30:57, dcheng wrote: > > > > > I'm a little confused by the ownership model of ScriptSourceCode. It > > > doesn't > > > > > inherit from GarbageCollected or GarbageCollectedFinalized, but it has > > > Member > > > > > variables. How does this work? +haraken > > > > > > > > Yes, because ScriptSourceCode is used only as a part-of object in a > > > > GarbageCollected object. > > > > > > > > The owner GarbageCollected object needs to trace ScriptSourceCode. Then > > > > ScriptSourceCode's members are traced. > > > > > > I see. How come Oilpan doesn't warn that the holder class here isn't GCed > > > though? It's allocated and stored in a unique_ptr, which should be an error, > > > right? > > > > ScriptSourceCode is in HeapVector (i.e., a GCed object). So this is fine. > > So just to confirm, we have the following ownership: > > SuspendableScriptExecutor (GarbageCollectedFinalized) > std::unique_ptr<SuspendableScriptExecutor::Executor> (Executor is not GCed) > HeapVector<ScriptSourceCode> (ScriptSourceCode is not GCed) > Member<ScriptResource>, Member<ScriptStreamer>, etc... > (GarbageCollectedFinalized) > > Though both SuspendableScriptExecutor::Executor and ScriptSourceCode are both > not GCed, they define trace() methods, so this is OK, because Oilpan can still > reach the garbage collected members stored inside them. Is this correct? Exactly, that's correct :)
On 2016/09/28 00:34:56, haraken wrote: > On 2016/09/27 22:24:25, dcheng wrote: > > > https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): > > > > > https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:30: > > HeapVector<ScriptSourceCode> m_sources; > > On 2016/09/27 13:25:41, haraken wrote: > > > On 2016/09/27 09:02:59, dcheng wrote: > > > > On 2016/09/27 08:39:45, haraken wrote: > > > > > On 2016/09/27 08:30:57, dcheng wrote: > > > > > > I'm a little confused by the ownership model of ScriptSourceCode. It > > > > doesn't > > > > > > inherit from GarbageCollected or GarbageCollectedFinalized, but it has > > > > Member > > > > > > variables. How does this work? +haraken > > > > > > > > > > Yes, because ScriptSourceCode is used only as a part-of object in a > > > > > GarbageCollected object. > > > > > > > > > > The owner GarbageCollected object needs to trace ScriptSourceCode. Then > > > > > ScriptSourceCode's members are traced. > > > > > > > > I see. How come Oilpan doesn't warn that the holder class here isn't GCed > > > > though? It's allocated and stored in a unique_ptr, which should be an > error, > > > > right? > > > > > > ScriptSourceCode is in HeapVector (i.e., a GCed object). So this is fine. > > > > So just to confirm, we have the following ownership: > > > > SuspendableScriptExecutor (GarbageCollectedFinalized) > > std::unique_ptr<SuspendableScriptExecutor::Executor> (Executor is not GCed) > > HeapVector<ScriptSourceCode> (ScriptSourceCode is not GCed) > > Member<ScriptResource>, Member<ScriptStreamer>, etc... > > (GarbageCollectedFinalized) > > > > Though both SuspendableScriptExecutor::Executor and ScriptSourceCode are both > > not GCed, they define trace() methods, so this is OK, because Oilpan can still > > reach the garbage collected members stored inside them. Is this correct? > > Exactly, that's correct :) Oh, sorry, now I understand what you mean. That's not correct. Who traces the HeapVector<ScriptSourceCode>? Given that Executor is not GarbageCollected, we need to use PersistentHeapVector<ScriptSourceCode> or make Executor GarbageCollected.
These Oilpan errors should have been caught by the GC plugin -- but there's a bug that the GC plugin doesn't run the check for an inner class (the Executor class)... We need to fix it. https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:27: void trace(Visitor*) override; Use DEFINE_INLINE_VIRTUAL_TRACE. https://codereview.chromium.org/2339683006/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/SuspendableScriptExecutor.h (right): https://codereview.chromium.org/2339683006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/SuspendableScriptExecutor.h:31: class Executor { Make this inherit from GarbageCollected<>. https://codereview.chromium.org/2339683006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/SuspendableScriptExecutor.h:35: virtual void trace(Visitor*) {}; Use DEFINE_INLINE_VIRTUAL_TRACE. https://codereview.chromium.org/2339683006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/SuspendableScriptExecutor.h:52: std::unique_ptr<Executor> m_executor; Use Member<Executor>.
yhirano@chromium.org changed reviewers: + yhirano@chromium.org
https://codereview.chromium.org/2339683006/diff/60001/extensions/renderer/scr... File extensions/renderer/script_context.cc (right): https://codereview.chromium.org/2339683006/diff/60001/extensions/renderer/scr... extensions/renderer/script_context.cc:216: } else { On 2016/09/28 00:04:14, falken wrote: > On 2016/09/27 08:30:57, dcheng wrote: > > On 2016/09/16 18:16:31, Devlin wrote: > > > This can happen in the case of e.g. ServiceWorker contexts. What should we > do > > > here? > > > > Handwaving a bit... maybe this is OK? I don't think service workers can go > into > > nested run loops or do things that would cause script execution to be > > suspended... so in theory, we shouldn't up doing work in nested message loops > > that surprises code above it in the stack. > > > > +falken to confirm > > +nhiroki, +yhirano > > I probably don't understand enough to give an authoritative answer. dcheng tells > me sync APIs are the key to nested run loops. Service worker in principle > doesn't have sync APIs except for importScripts(). During the importScripts call > we wouldn't be dispatching an event to the SW, since the browser process waits > for the SW to finish starting up before it sends messages to the renderer to > dispatch events. Also currently FileReaderSync is available but that is a bug > <https://github.com/w3c/ServiceWorker/issues/735>. > > If it's relevant, SW also can't terminate itself synchronously: the browser > process orders the renderer to terminate the worker. This used to result in a > terminateExecution but it changed a bit recently in https://crbug.com/487050 to > have a more graceful shutdown. This topic is about sync loading on a worker thread, right? My understanding is it's not related with a nested message loop. Sync loading on a worker thread is implemented in WorkerThreadableLoader[1] and it blocks the worker thread completely. On the other hand, the main thread is working normally. 1: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Wo...
haraken@chromium.org changed reviewers: + yutak@chromium.org
dcheng/haraken, please see comment in SuspendableScriptExecutor.cpp. Sorry, my blink fu is nonexistent. :/ https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2339683006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:27: void trace(Visitor*) override; On 2016/09/28 00:41:22, haraken wrote: > > Use DEFINE_INLINE_VIRTUAL_TRACE. Done. https://codereview.chromium.org/2339683006/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/SuspendableScriptExecutor.h (right): https://codereview.chromium.org/2339683006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/SuspendableScriptExecutor.h:31: class Executor { On 2016/09/28 00:41:22, haraken wrote: > > Make this inherit from GarbageCollected<>. Done. https://codereview.chromium.org/2339683006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/SuspendableScriptExecutor.h:35: virtual void trace(Visitor*) {}; On 2016/09/28 00:41:22, haraken wrote: > > Use DEFINE_INLINE_VIRTUAL_TRACE. Done. https://codereview.chromium.org/2339683006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/SuspendableScriptExecutor.h:52: std::unique_ptr<Executor> m_executor; On 2016/09/28 00:41:22, haraken wrote: > > Use Member<Executor>. Done. https://codereview.chromium.org/2339683006/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2339683006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:112: SuspendableScriptExecutor* executor = new SuspendableScriptExecutor(frame, callback, new V8FunctionExecutor(isolate, function, receiver, argc, argv)); Compiler error here (and on line 106): "error: member 'operator new' found in multiple base classes of different types" - what should we be doing for garbage collected on V8FunctionExecutor/WebScriptExecutor?
https://codereview.chromium.org/2339683006/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2339683006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:22: class WebScriptExecutor : public GarbageCollectedFinalized<WebScriptExecutor>, public SuspendableScriptExecutor::Executor { WebScriptExecutor should not inherit from GarbageCollectedFinalized, because SuspendableScriptExecutor::Executor already inherits from GarbageCollected. Just make SuspendableScriptExecutor::Executor inherit from GarbageCollectedFinalized. And make WebScriptExecutor not inherit from GarbageCollectedFinalized. https://codereview.chromium.org/2339683006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:66: class V8FunctionExecutor : public GarbageCollectedFinalized<V8FunctionExecutor>, public SuspendableScriptExecutor::Executor { Ditto. https://codereview.chromium.org/2339683006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:112: SuspendableScriptExecutor* executor = new SuspendableScriptExecutor(frame, callback, new V8FunctionExecutor(isolate, function, receiver, argc, argv)); On 2016/09/30 21:22:22, Devlin (catching up) wrote: > Compiler error here (and on line 106): "error: member 'operator new' found in > multiple base classes of different types" - what should we be doing for garbage > collected on V8FunctionExecutor/WebScriptExecutor? See my comment in the above. https://codereview.chromium.org/2339683006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:112: SuspendableScriptExecutor* executor = new SuspendableScriptExecutor(frame, callback, new V8FunctionExecutor(isolate, function, receiver, argc, argv)); BTW, is SuspendableScriptExecutor used only on stack? Then it would be easier to make it as STACK_ALLOCATED (instead of making it GarbageCollected).
https://codereview.chromium.org/2339683006/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2339683006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:22: class WebScriptExecutor : public GarbageCollectedFinalized<WebScriptExecutor>, public SuspendableScriptExecutor::Executor { On 2016/10/03 02:30:09, haraken wrote: > > WebScriptExecutor should not inherit from GarbageCollectedFinalized, because > SuspendableScriptExecutor::Executor already inherits from GarbageCollected. > > Just make SuspendableScriptExecutor::Executor inherit from > GarbageCollectedFinalized. And make WebScriptExecutor not inherit from > GarbageCollectedFinalized. Done. https://codereview.chromium.org/2339683006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:66: class V8FunctionExecutor : public GarbageCollectedFinalized<V8FunctionExecutor>, public SuspendableScriptExecutor::Executor { On 2016/10/03 02:30:09, haraken wrote: > > Ditto. Done. https://codereview.chromium.org/2339683006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:112: SuspendableScriptExecutor* executor = new SuspendableScriptExecutor(frame, callback, new V8FunctionExecutor(isolate, function, receiver, argc, argv)); On 2016/10/03 02:30:09, haraken wrote: > > BTW, is SuspendableScriptExecutor used only on stack? Then it would be easier to > make it as STACK_ALLOCATED (instead of making it GarbageCollected). SuspendableScriptExecutor is only used on the heap, unless I'm missing something. We create it here and it can finish either synchronously (in which case it's freed right away) or asynchronously (in which case it's freed once it executes).
https://codereview.chromium.org/2339683006/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2339683006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:112: SuspendableScriptExecutor* executor = new SuspendableScriptExecutor(frame, callback, new V8FunctionExecutor(isolate, function, receiver, argc, argv)); On 2016/10/03 20:53:30, Devlin wrote: > On 2016/10/03 02:30:09, haraken wrote: > > > > BTW, is SuspendableScriptExecutor used only on stack? Then it would be easier > to > > make it as STACK_ALLOCATED (instead of making it GarbageCollected). > > SuspendableScriptExecutor is only used on the heap, unless I'm missing > something. We create it here and it can finish either synchronously (in which > case it's freed right away) or asynchronously (in which case it's freed once it > executes). My question is if you can write the code like: SuspendableScriptExecutor executor(...); executor->run(); Then SuspendableScriptExecutor is stack-allocated. You can mark it as STACK_ALLOCATED and don't need to make it GarbageCollected.
https://codereview.chromium.org/2339683006/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2339683006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:112: SuspendableScriptExecutor* executor = new SuspendableScriptExecutor(frame, callback, new V8FunctionExecutor(isolate, function, receiver, argc, argv)); On 2016/10/04 00:08:30, haraken wrote: > On 2016/10/03 20:53:30, Devlin wrote: > > On 2016/10/03 02:30:09, haraken wrote: > > > > > > BTW, is SuspendableScriptExecutor used only on stack? Then it would be > easier > > to > > > make it as STACK_ALLOCATED (instead of making it GarbageCollected). > > > > SuspendableScriptExecutor is only used on the heap, unless I'm missing > > something. We create it here and it can finish either synchronously (in which > > case it's freed right away) or asynchronously (in which case it's freed once > it > > executes). > > My question is if you can write the code like: > > SuspendableScriptExecutor executor(...); > executor->run(); > > Then SuspendableScriptExecutor is stack-allocated. You can mark it as > STACK_ALLOCATED and don't need to make it GarbageCollected. The SuspendableScriptExecutor needs to be able to live outside the current scope, since the scripts may finish asynchronously (which is the reason to use SuspendableScriptExecutor instead of just directly executing the script immediately). As far as I know, there's no way we can do that with a stack-allocated object.
On 2016/10/04 00:14:26, Devlin wrote: > https://codereview.chromium.org/2339683006/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): > > https://codereview.chromium.org/2339683006/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:112: > SuspendableScriptExecutor* executor = new SuspendableScriptExecutor(frame, > callback, new V8FunctionExecutor(isolate, function, receiver, argc, argv)); > On 2016/10/04 00:08:30, haraken wrote: > > On 2016/10/03 20:53:30, Devlin wrote: > > > On 2016/10/03 02:30:09, haraken wrote: > > > > > > > > BTW, is SuspendableScriptExecutor used only on stack? Then it would be > > easier > > > to > > > > make it as STACK_ALLOCATED (instead of making it GarbageCollected). > > > > > > SuspendableScriptExecutor is only used on the heap, unless I'm missing > > > something. We create it here and it can finish either synchronously (in > which > > > case it's freed right away) or asynchronously (in which case it's freed once > > it > > > executes). > > > > My question is if you can write the code like: > > > > SuspendableScriptExecutor executor(...); > > executor->run(); > > > > Then SuspendableScriptExecutor is stack-allocated. You can mark it as > > STACK_ALLOCATED and don't need to make it GarbageCollected. > > The SuspendableScriptExecutor needs to be able to live outside the current > scope, since the scripts may finish asynchronously (which is the reason to use > SuspendableScriptExecutor instead of just directly executing the script > immediately). As far as I know, there's no way we can do that with a > stack-allocated object. Ah, makes sense. web/ LGTM https://codereview.chromium.org/2339683006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2339683006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:104: v8::Isolate* isolate = v8::Isolate::GetCurrent(); Isolate::GetCurrent() is deprecated. Use toIsolate(frame). https://codereview.chromium.org/2339683006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:137: v8::Isolate* isolate, Move the Isolate* parameter to the first arg.
Just one major remaining question about this CL about service workers, due to the TODO. https://codereview.chromium.org/2339683006/diff/120001/extensions/renderer/sc... File extensions/renderer/script_context.cc (right): https://codereview.chromium.org/2339683006/diff/120001/extensions/renderer/sc... extensions/renderer/script_context.cc:216: // TODO(devlin): This probably isn't safe. Hmm, is it possible to attach devtools to a service worker? I'm just trying to think of the normal ways we would get script re-entrancy. If devtools is attached, how do we suspend script execution in the SW? https://codereview.chromium.org/2339683006/diff/120001/extensions/renderer/sc... File extensions/renderer/script_context.h (right): https://codereview.chromium.org/2339683006/diff/120001/extensions/renderer/sc... extensions/renderer/script_context.h:123: // TODO(devlin): Remove the above variants in favor of this. Btw, do you estimate that this will be a lot of work if we're just trying to fix extension code?
https://codereview.chromium.org/2339683006/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebLocalFrame.h (right): https://codereview.chromium.org/2339683006/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/web/WebLocalFrame.h:193: virtual void requestExecuteV8Function(v8::Local<v8::Function>, Also, I forgot to save this comment the first time, but should we mark the other function (callFunctionEvenIfScriptDisabled on WebFrame) as deprecated as well?
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
https://codereview.chromium.org/2339683006/diff/120001/extensions/renderer/sc... File extensions/renderer/script_context.cc (right): https://codereview.chromium.org/2339683006/diff/120001/extensions/renderer/sc... extensions/renderer/script_context.cc:216: // TODO(devlin): This probably isn't safe. On 2016/10/04 05:58:49, dcheng wrote: > Hmm, is it possible to attach devtools to a service worker? I'm just trying to > think of the normal ways we would get script re-entrancy. If devtools is > attached, how do we suspend script execution in the SW? That sounds like a question for falken. :) Regardless, we'd definitely need something different, since SWs don't have associated frames. https://codereview.chromium.org/2339683006/diff/120001/extensions/renderer/sc... File extensions/renderer/script_context.h (right): https://codereview.chromium.org/2339683006/diff/120001/extensions/renderer/sc... extensions/renderer/script_context.h:123: // TODO(devlin): Remove the above variants in favor of this. On 2016/10/04 05:58:49, dcheng wrote: > Btw, do you estimate that this will be a lot of work if we're just trying to fix > extension code? It's hard to say until I really start in on it. I think I ran into at least a few cases where we relied on the result, which would need to be restructured. I'm hoping there aren't any where we synchronously return a result to an extension, because that would be very difficult indeed. https://codereview.chromium.org/2339683006/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebLocalFrame.h (right): https://codereview.chromium.org/2339683006/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/web/WebLocalFrame.h:193: virtual void requestExecuteV8Function(v8::Local<v8::Function>, On 2016/10/04 05:59:53, dcheng wrote: > Also, I forgot to save this comment the first time, but should we mark the other > function (callFunctionEvenIfScriptDisabled on WebFrame) as deprecated as well? I'm happy to. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
LGTM, but let's test and make sure this works as advertised before we land. https://codereview.chromium.org/2339683006/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebLocalFrame.h (right): https://codereview.chromium.org/2339683006/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/web/WebLocalFrame.h:187: // DEPRECATED: Prefer requestExecuteScriptInIsolatedWorld(). Sorry, I was unclear: I meant WebFrame::callFunctionEvenIfScriptDisabled in https://cs.chromium.org/chromium/src/third_party/WebKit/public/web/WebFrame.h...: should we mark that as deprecated and point to th enewly added function below?
https://codereview.chromium.org/2339683006/diff/120001/extensions/renderer/sc... File extensions/renderer/script_context.cc (right): https://codereview.chromium.org/2339683006/diff/120001/extensions/renderer/sc... extensions/renderer/script_context.cc:216: // TODO(devlin): This probably isn't safe. On 2016/10/04 19:40:05, Devlin wrote: > On 2016/10/04 05:58:49, dcheng wrote: > > Hmm, is it possible to attach devtools to a service worker? I'm just trying to > > think of the normal ways we would get script re-entrancy. If devtools is > > attached, how do we suspend script execution in the SW? > > That sounds like a question for falken. :) Regardless, we'd definitely need > something different, since SWs don't have associated frames. Yes, you can attach DevTools to service workers and set breakpoints etc. +dgozman.
dgozman@chromium.org changed reviewers: + dgozman@chromium.org
https://codereview.chromium.org/2339683006/diff/120001/extensions/renderer/sc... File extensions/renderer/script_context.cc (right): https://codereview.chromium.org/2339683006/diff/120001/extensions/renderer/sc... extensions/renderer/script_context.cc:216: // TODO(devlin): This probably isn't safe. On 2016/10/05 00:19:55, falken wrote: > On 2016/10/04 19:40:05, Devlin wrote: > > On 2016/10/04 05:58:49, dcheng wrote: > > > Hmm, is it possible to attach devtools to a service worker? I'm just trying > to > > > think of the normal ways we would get script re-entrancy. If devtools is > > > attached, how do we suspend script execution in the SW? > > > > That sounds like a question for falken. :) Regardless, we'd definitely need > > something different, since SWs don't have associated frames. > > Yes, you can attach DevTools to service workers and set breakpoints etc. > +dgozman. This should be fine as long as you execute JS via SuspendableScriptExecutor, which is an ActiveDOMObject. We pause all of these while on breakpoint.
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Patchset #6 (id:160001) has been deleted
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Any objections to me landing this? (asking because there are a lot of reviewers, but it's unclear how many I should be waiting for lg's from)
I'm a little concerned about the SW path, but I think we should land this and figure out how to handle this correctly for paused SW.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/06 00:25:07, dcheng wrote: > I'm a little concerned about the SW path, but I think we should land this and > figure out how to handle this correctly for paused SW. Sounds good. Landing.
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2339683006/#ps180001 (title: "Win compile fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Blink] Modify SuspendableScriptExecutor to take a v8::Function Allow SuspendableScriptExecutor to take a v8::Function so that it can be used from Extensions code. Also wire up WebLocalFrame and ScriptContext to have handles to use it. Convert a couple of basic extensions call sites as a POC. BUG=629431 ========== to ========== [Blink] Modify SuspendableScriptExecutor to take a v8::Function Allow SuspendableScriptExecutor to take a v8::Function so that it can be used from Extensions code. Also wire up WebLocalFrame and ScriptContext to have handles to use it. Convert a couple of basic extensions call sites as a POC. BUG=629431 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [Blink] Modify SuspendableScriptExecutor to take a v8::Function Allow SuspendableScriptExecutor to take a v8::Function so that it can be used from Extensions code. Also wire up WebLocalFrame and ScriptContext to have handles to use it. Convert a couple of basic extensions call sites as a POC. BUG=629431 ========== to ========== [Blink] Modify SuspendableScriptExecutor to take a v8::Function Allow SuspendableScriptExecutor to take a v8::Function so that it can be used from Extensions code. Also wire up WebLocalFrame and ScriptContext to have handles to use it. Convert a couple of basic extensions call sites as a POC. BUG=629431 Committed: https://crrev.com/a9bc8ccd2becc952e4cea2d8a053247a481dd483 Cr-Commit-Position: refs/heads/master@{#423548} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a9bc8ccd2becc952e4cea2d8a053247a481dd483 Cr-Commit-Position: refs/heads/master@{#423548}
Message was sent while issue was closed.
@redvlin, PTAL https://crbug.com/754976 - bisecting and debugging points to this CL.
Message was sent while issue was closed.
sorry for the typo, I meant @rdevlin |
