Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1167343002:
Add methods for creating V8 extras-based ReadableStreams from C++ (Closed)
|
Created:
5 years, 6 months ago by domenic Modified:
4 years, 10 months ago CC:
arv+blink, blink-reviews, blink-reviews-bindings_chromium.org, Inactive, tyoshino (SeeGerritForStatus), vivekg_samsung, vivekg Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds ReadableStreamOperations::{createReadableStream,createCountQueuingStrategy}, as well as the supporting UnderlyingSourceBase and ReadableStreamController classes. The test demonstrates how to use these.
This revealed a circular dependency between ToV8.h and ScriptValue.h that had to be resolved by moving one of the ScriptValue::from overloads into ToV8.h. While doing so, convert it to use perfect forwarding.
R=yhirano@chromium.org,haraken@chromium.org
BUG=503491
Committed: https://crrev.com/81c4571248f30446f95901a76a5059929b467434
Cr-Commit-Position: refs/heads/master@{#374308}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Remove C++ queuing strategies #
Total comments: 1
Patch Set 3 : Use Oilpan more #Patch Set 4 : Rebase on recent V8/Blink changes #Patch Set 5 : #Patch Set 6 : Make USB ActiveDOMObject. Add second readable stream to internals, with exposed controller #Patch Set 7 : Linker errors on this new testing strategy #Patch Set 8 : Linker and compiler errors fixed yay #Patch Set 9 : Tests pass! ActiveDOMObject => finalized, also #Patch Set 10 : Weak pointer, but crashing because of not clearing (I think) #Patch Set 11 : Take care of some TODOs; remove attempt at cancel #Patch Set 12 : That todo is not necessary according to yhirano@ #Patch Set 13 : Port old work on top of new work, minus tests for now #Patch Set 14 : Add tests; change to ScriptValue everywhere #Patch Set 15 : Don't modify UnderlyingSource.h, oops #
Total comments: 1
Patch Set 16 : Rebase on smaller CLs; move ReadableStreamController #
Total comments: 23
Patch Set 17 : Address most review comments #
Total comments: 5
Patch Set 18 : Add ActiveDOMObject check #
Total comments: 11
Patch Set 19 : Address review comments #Patch Set 20 : Properly rebase #Patch Set 21 : Minor tweaks #
Total comments: 3
Patch Set 22 : Add comments saying context must be valid #Patch Set 23 : rebase on master #Patch Set 24 : Fix error that clang catches but MSVS does not #Patch Set 25 : Rebase on master; clang/win compiles without issue, uh oh #Patch Set 26 : Trying blindly to resolve circular dependency #
Total comments: 1
Messages
Total messages: 83 (24 generated)
https://codereview.chromium.org/1167343002/diff/1/Source/core/streams/Queuing... File Source/core/streams/QueuingStrategyBase.idl (right): https://codereview.chromium.org/1167343002/diff/1/Source/core/streams/Queuing... Source/core/streams/QueuingStrategyBase.idl:13: double size(any chunk); This is broken right now, causing the layout tests to fail, since EnqueueInReadableStream fails. I am not sure how to fix it. The problem is that the stream spec is written to treat size as a function, not a method. That is, it saves a copy `stream@[[strategySize]] = strategy.size` and later calls it directly, with an undefined `this`. But all bindings-generated methods check their `this`. I am not sure how to do this best in Blink. Maybe queuing strategies should always be implemented as V8 extras, and/or use one of the built-in ones (ByteLengthQueuingStrategy/CountQueuingStrategy)? Or is there some way of making our bindings not do the `this`-checking?
In patch set 2 I stopped allowing C++ queuing strategies, in favor of implementing them via V8 extras. Now the layout tests pass, which is great news! I'm adding back the CCs that I accidentally deleted... still not sure what the etiquette is here, especially for WIP CLs. This still needs a lot of review.
Sorry, I still don't understand how you resolve the reference cycle problem. Can you explain? Are the following correct? - ReadableByteStream2 has a strong reference to JSReadableByteStream\ - JSReadableByteStream has a strong reference to JSUnderlyingSource - JSUnderlyingSource has a strong reference to UnderlyingSource - UnderlyingSource subclass has a strong reference to ReadableByteStreamController. - ReadableByteStreamController has a strong reference to ReadableByteStream2. https://codereview.chromium.org/1167343002/diff/20001/Source/core/streams/Und... File Source/core/streams/UnderlyingSourceBase.h (right): https://codereview.chromium.org/1167343002/diff/20001/Source/core/streams/Und... Source/core/streams/UnderlyingSourceBase.h:33: ReadableStreamController* m_controller; Can we have a controller of type ReadableStreamController, not a pointer?
Not quite. See the diagram in https://docs.google.com/document/d/1HqZctD-aHSbALCxl_AKSkvQ9g95DR8CQStdBYiCC2.... But it is basically correct. The only difference is ReadableStreamController has a strong reference to JSReadableStream. (Also you inserted "byte" in all of those but we aren't doing byte streams yet.) tyoshino@'s work in https://github.com/whatwg/streams/pull/361#discussion_r32265033 points out that maybe we can move most of the interesting stuff into ReadableStreamController. That would mean we don't need a reference to the ReadableStream from the controller. Instead the controller would just toV8 itself and use that. It is OK for there to be a mutual reference between ReadableStreamController and JSReadableStreamController, right? Actually, now that I think about it, we could implement this without as much work by exposing a way to get readableStreamControllerControlledReadableStream. Then the ReadableStreamController could toV8 itself and use that. I will try that first. On Thu, Jun 11, 2015 at 11:41 PM <yhirano@chromium.org> wrote: > Sorry, I still don't understand how you resolve the reference cycle > problem. Can > you explain? > > Are the following correct? > - ReadableByteStream2 has a strong reference to JSReadableByteStream\ > - JSReadableByteStream has a strong reference to JSUnderlyingSource > - JSUnderlyingSource has a strong reference to UnderlyingSource > - UnderlyingSource subclass has a strong reference to > ReadableByteStreamController. > - ReadableByteStreamController has a strong reference to > ReadableByteStream2. > > > > > https://codereview.chromium.org/1167343002/diff/20001/Source/core/streams/Und... > File Source/core/streams/UnderlyingSourceBase.h (right): > > > https://codereview.chromium.org/1167343002/diff/20001/Source/core/streams/Und... > Source/core/streams/UnderlyingSourceBase.h:33 > <https://codereview.chromium.org/1167343002/diff/20001/Source/core/streams/Und...>: > ReadableStreamController* > m_controller; > Can we have a controller of type ReadableStreamController, not a > pointer? > > https://codereview.chromium.org/1167343002/ > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Wait, I am sorry, that is not right. There is no JSReadableStreamController right now. The diagram is wrong too. UA-created JSReadableStream's have null for their JS controller reference; it is kept instead by the UnderlyingSource subclass. I will update the diagram at least. On Tue, Jun 16, 2015 at 4:48 PM Domenic Denicola <domenic@google.com> wrote: > Not quite. See the diagram in > https://docs.google.com/document/d/1HqZctD-aHSbALCxl_AKSkvQ9g95DR8CQStdBYiCC2.... > But it is basically correct. The only difference is ReadableStreamController > has a strong reference to JSReadableStream. (Also you inserted "byte" in > all of those but we aren't doing byte streams yet.) > > tyoshino@'s work in > https://github.com/whatwg/streams/pull/361#discussion_r32265033 points > out that maybe we can move most of the interesting stuff into > ReadableStreamController. That would mean we don't need a reference to the > ReadableStream from the controller. Instead the controller would just toV8 > itself and use that. It is OK for there to be a mutual reference between > ReadableStreamController and JSReadableStreamController, right? > > Actually, now that I think about it, we could implement this without as > much work by exposing a way to > get readableStreamControllerControlledReadableStream. Then the ReadableStreamController > could toV8 itself and use that. I will try that first. > > On Thu, Jun 11, 2015 at 11:41 PM <yhirano@chromium.org> wrote: > >> Sorry, I still don't understand how you resolve the reference cycle >> problem. Can >> you explain? >> >> Are the following correct? >> - ReadableByteStream2 has a strong reference to JSReadableByteStream\ >> - JSReadableByteStream has a strong reference to JSUnderlyingSource >> - JSUnderlyingSource has a strong reference to UnderlyingSource >> - UnderlyingSource subclass has a strong reference to >> ReadableByteStreamController. >> - ReadableByteStreamController has a strong reference to >> ReadableByteStream2. >> >> >> >> >> https://codereview.chromium.org/1167343002/diff/20001/Source/core/streams/Und... >> File Source/core/streams/UnderlyingSourceBase.h (right): >> >> >> https://codereview.chromium.org/1167343002/diff/20001/Source/core/streams/Und... >> Source/core/streams/UnderlyingSourceBase.h:33 >> <https://codereview.chromium.org/1167343002/diff/20001/Source/core/streams/Und...>: >> ReadableStreamController* >> m_controller; >> Can we have a controller of type ReadableStreamController, not a >> pointer? >> >> https://codereview.chromium.org/1167343002/ >> > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
I've uploaded a new changeset which makes all objects involved GarbageCollected<>. So for now m_controller in UnderlyingSourceBase is a Member<ReadableStreamController>. yhirano@, you recommended ReadableStreamController instead---maybe that is better? The reference cycle is now ReadableStream2 -> JSReadableStream -> JSUnderlyingSource -> UnderlyingSourceBase subclass -> (via Member<>) ReadableStreamController -> JSReadableStream. Note that the ReadableStreamController -> JSReadableStream reference gets cut if you call the controller's close() or error() methods. Maybe that is enough to prevent any problems? On Tue, Jun 16, 2015 at 5:03 PM Domenic Denicola <domenic@google.com> wrote: > Wait, I am sorry, that is not right. There is no > JSReadableStreamController right now. The diagram is wrong too. UA-created > JSReadableStream's have null for their JS controller reference; it is kept > instead by the UnderlyingSource subclass. > > I will update the diagram at least. > > > On Tue, Jun 16, 2015 at 4:48 PM Domenic Denicola <domenic@google.com> > wrote: > >> Not quite. See the diagram in >> https://docs.google.com/document/d/1HqZctD-aHSbALCxl_AKSkvQ9g95DR8CQStdBYiCC2.... >> But it is basically correct. The only difference is ReadableStreamController >> has a strong reference to JSReadableStream. (Also you inserted "byte" in >> all of those but we aren't doing byte streams yet.) >> >> tyoshino@'s work in >> https://github.com/whatwg/streams/pull/361#discussion_r32265033 points >> out that maybe we can move most of the interesting stuff into >> ReadableStreamController. That would mean we don't need a reference to the >> ReadableStream from the controller. Instead the controller would just toV8 >> itself and use that. It is OK for there to be a mutual reference between >> ReadableStreamController and JSReadableStreamController, right? >> >> Actually, now that I think about it, we could implement this without as >> much work by exposing a way to >> get readableStreamControllerControlledReadableStream. Then the ReadableStreamController >> could toV8 itself and use that. I will try that first. >> >> On Thu, Jun 11, 2015 at 11:41 PM <yhirano@chromium.org> wrote: >> >>> Sorry, I still don't understand how you resolve the reference cycle >>> problem. Can >>> you explain? >>> >>> Are the following correct? >>> - ReadableByteStream2 has a strong reference to JSReadableByteStream\ >>> - JSReadableByteStream has a strong reference to JSUnderlyingSource >>> - JSUnderlyingSource has a strong reference to UnderlyingSource >>> - UnderlyingSource subclass has a strong reference to >>> ReadableByteStreamController. >>> - ReadableByteStreamController has a strong reference to >>> ReadableByteStream2. >>> >>> >>> >>> >>> https://codereview.chromium.org/1167343002/diff/20001/Source/core/streams/Und... >>> File Source/core/streams/UnderlyingSourceBase.h (right): >>> >>> >>> https://codereview.chromium.org/1167343002/diff/20001/Source/core/streams/Und... >>> Source/core/streams/UnderlyingSourceBase.h:33 >>> <https://codereview.chromium.org/1167343002/diff/20001/Source/core/streams/Und...>: >>> ReadableStreamController* >>> m_controller; >>> Can we have a controller of type ReadableStreamController, not a >>> pointer? >>> >>> https://codereview.chromium.org/1167343002/ >>> >> To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Created a bug entry for this work. http://crbug.com/503491 Please use this for BUG= line. Thanks
Sorry for the late response. > Note that the ReadableStreamController -> JSReadableStream reference gets > cut if you call the controller's close() or error() methods. Maybe that is > enough to prevent any problems? I'd like to collect the stream + source when the stream is not referenced and not locked even when it is readable. Example: fetch(url);
The CQ bit was checked by domenic@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1167343002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Hi Domenic, IIRC you said you would land this change (i.e. introducing UnderlayingSourceBase, etc) when we last met, right?
On 2016/01/07 at 08:26:36, yhirano wrote: > Hi Domenic, IIRC you said you would land this change (i.e. introducing UnderlayingSourceBase, etc) when we last met, right? Yes! Sorry for the delay; I have been distracted by modules-spec related stuff. I will try to spend some time on it this week and early next week.
On 2016/01/07 at 16:37:36, domenic wrote: > On 2016/01/07 at 08:26:36, yhirano wrote: > > Hi Domenic, IIRC you said you would land this change (i.e. introducing UnderlayingSourceBase, etc) when we last met, right? > > Yes! Sorry for the delay; I have been distracted by modules-spec related stuff. I will try to spend some time on it this week and early next week. I've uploaded an updated patch set, but it isn't really ready for review yet (missing tests, haven't even tried compiling it). I wanted to just ask a few questions first based on that work: - I put createReadableStream and createCountQueuingStrategy in ReadableStreamOperations.cpp since that seemed like the right place based on your earlier commit at https://blink.lc/chromium/commit/third_party/WebKit?id=e1cf4a50c55b6ebac10c9c.... But, I realize this introduces a dependency from bindings/core/v8 to core/streams (since it needs UnderlyingSourceBase). Is that OK? - Should I port over the tests from my previous patch set, or try to do C++ tests like you did in your commit, or both? My previous patch set's tests were integration-style tests including about GC. They seem pretty valuable but maybe I should add some C++ tests too? - I like using auto but that doesn't seem to be the prevailing style in core/streams or bindings/core/v8. Should I change to explicit types everywhere? Thanks. Will spend more time on this tomorrow and maybe later tonight.
On 2016/01/14 00:12:37, domenic wrote: > On 2016/01/07 at 16:37:36, domenic wrote: > > On 2016/01/07 at 08:26:36, yhirano wrote: > > > Hi Domenic, IIRC you said you would land this change (i.e. introducing > UnderlayingSourceBase, etc) when we last met, right? > > > > Yes! Sorry for the delay; I have been distracted by modules-spec related > stuff. I will try to spend some time on it this week and early next week. > > I've uploaded an updated patch set, but it isn't really ready for review yet > (missing tests, haven't even tried compiling it). I wanted to just ask a few > questions first based on that work: > > - I put createReadableStream and createCountQueuingStrategy in > ReadableStreamOperations.cpp since that seemed like the right place based on > your earlier commit at > https://blink.lc/chromium/commit/third_party/WebKit?id=e1cf4a50c55b6ebac10c9c.... > But, I realize this introduces a dependency from bindings/core/v8 to > core/streams (since it needs UnderlyingSourceBase). Is that OK? > I think it's OK. ReadableOperations is a kind of custom binding and generated binding code includes related core headers (See https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin... for example). > - Should I port over the tests from my previous patch set, or try to do C++ > tests like you did in your commit, or both? My previous patch set's tests were > integration-style tests including about GC. They seem pretty valuable but maybe > I should add some C++ tests too? > If you prefer C++ unit tests, in this case not having layout tests is OK. If you prefer layout tests, please write at least one (simple) C++ unit test for each public ReadableStreamOperations methods you add. Please test detailed cases in layout tests. > - I like using auto but that doesn't seem to be the prevailing style in > core/streams or bindings/core/v8. Should I change to explicit types everywhere? > Have you ever read https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/5-Bt3BJzAo0? My takeaway from the thread is "Do not use auto for smart pointers except in ranged-for". Smart pointers include Oilpan related pointers. I prefer being explicit about v8::Local<>, but I don't have a strong opinion here - v8 people may have.
Description was changed from
==========
[WIP] Initial attempt at V8-extras-based ReadableStream
Includes some basic layout tests.
Depends on the following modification to Chromium's build/common.gypi:
@@ -1479,6 +1479,11 @@
# Compile d8 for the host toolset.
'v8_toolset_for_d8': 'host',
+ # Add the streams V8 extension to the snapshot
+ 'v8_extra_library_files': [
+ '../third_party/WebKit/Source/core/streams/ByteLengthQueuingStrategy.js',
+ '../third_party/WebKit/Source/core/streams/CountQueuingStrategy.js',
+ '../third_party/WebKit/Source/core/streams/ReadableStream2.js',
+ ],
+
# Use brlapi from brltty for braille display support.
'use_brlapi%': 0,
R=tyoshino@chromium.org, yhirano@chromium.org, haraken@chromium.org
BUG=
==========
to
==========
Adds
ReadableStreamOperations::{createReadableStream,createCountQueuingStrategy}, as
well as the supporting UnderlyingSourceBase and ReadableStreamController
classes. The test demonstrates how to use these.
Along the way:
- Converts other ReadableStreamOperations methods to use ScriptValue instead of
sometimes using v8::Local<v8::Value>
- Moved some V8 extras utility functions to V8BindingMacros.h
R=tyoshino@chromium.org, yhirano@chromium.org, haraken@chromium.org
BUG=503491
==========
Uploaded CL is ready for review. Also updated title and description. One thing I did was convert the existing ReadableStreamOperations methods to use ScriptValue instead of v8::Local<v8::Value>. Let me know if you would prefer that be a separate CL that this builds on top of. I could even do a third CL for moving the V8 extras call utilities into V8BindingMacros.h.
> Uploaded CL is ready for review. Also updated title and description. > > One thing I did was convert the existing ReadableStreamOperations methods to use > ScriptValue instead of v8::Local<v8::Value>. Let me know if you would prefer > that be a separate CL that this builds on top of. I could even do a third CL for > moving the V8 extras call utilities into V8BindingMacros.h. I'm a fan of making CL size smaller :) https://codereview.chromium.org/1167343002/diff/270001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/streams/ReadableStreamController.h (right): https://codereview.chromium.org/1167343002/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/core/streams/ReadableStreamController.h:18: class CORE_EXPORT ReadableStreamController final : public GarbageCollectedFinalized<ReadableStreamController> { I think we should move this file to bindings/core/v8/. Classes that use direct V8 APIs need to be put in bindings/.
On 2016/01/15 at 03:06:33, haraken wrote: > > Uploaded CL is ready for review. Also updated title and description. > > > > One thing I did was convert the existing ReadableStreamOperations methods to use > > ScriptValue instead of v8::Local<v8::Value>. Let me know if you would prefer > > that be a separate CL that this builds on top of. I could even do a third CL for > > moving the V8 extras call utilities into V8BindingMacros.h. > > I'm a fan of making CL size smaller :) Split out a number of prerequisite CLs: - https://codereview.chromium.org/1591033002 - https://codereview.chromium.org/1594653003 - https://codereview.chromium.org/1595713003 Will revisit this CL on Monday, rebasing it on top of those.
Description was changed from
==========
Adds
ReadableStreamOperations::{createReadableStream,createCountQueuingStrategy}, as
well as the supporting UnderlyingSourceBase and ReadableStreamController
classes. The test demonstrates how to use these.
Along the way:
- Converts other ReadableStreamOperations methods to use ScriptValue instead of
sometimes using v8::Local<v8::Value>
- Moved some V8 extras utility functions to V8BindingMacros.h
R=tyoshino@chromium.org, yhirano@chromium.org, haraken@chromium.org
BUG=503491
==========
to
==========
Adds
ReadableStreamOperations::{createReadableStream,createCountQueuingStrategy}, as
well as the supporting UnderlyingSourceBase and ReadableStreamController
classes. The test demonstrates how to use these.
R=tyoshino@chromium.org,yhirano@chromium.org,haraken@chromium.org
BUG=503491
==========
Description was changed from
==========
Adds
ReadableStreamOperations::{createReadableStream,createCountQueuingStrategy}, as
well as the supporting UnderlyingSourceBase and ReadableStreamController
classes. The test demonstrates how to use these.
R=tyoshino@chromium.org,yhirano@chromium.org,haraken@chromium.org
BUG=503491
==========
to
==========
Adds
ReadableStreamOperations::{createReadableStream,createCountQueuingStrategy}, as
well as the supporting UnderlyingSourceBase and ReadableStreamController
classes. The test demonstrates how to use these.
R=yhirano@chromium.org,haraken@chromium.org
BUG=503491
==========
domenic@chromium.org changed reviewers: - tyoshino@chromium.org
On 2016/01/16 at 00:17:00, domenic wrote: > Will revisit this CL on Monday, rebasing it on top of those. Took me a bit more time than expected but rebased CL is now up and ready for review. Moved ReadableStreamController.h to the bindings directory per haraken@'s comment.
Mostly looks good. https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ReadableStreamController.h (right): https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ReadableStreamController.h:51: v8CallExtraOrCrash(m_scriptState, "CloseReadableStream", args); I haven't yet checked if we should add a MicroTaskSuppressionScope. If this is an internal function (i.e., not user script), we need to add a MicroTaskSuppressionScope. https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ReadableStreamController.h:105: ScriptState* m_scriptState; This should be RefPtr<ScriptState>. Otherwise it can go stale. https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.cpp (right): https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.cpp:23: V8RecursionScope::MicrotaskSuppression mtsScope(scriptState->isolate()); Maybe it would make sense to create another version of v8CallExtraOrCrash for calling an internal function. - callV8ExtraFunction() - callInternalV8ExtraFunction() https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/streams/UnderlyingSourceBase.cpp (right): https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou... third_party/WebKit/Source/core/streams/UnderlyingSourceBase.cpp:51: return m_controller && m_controller->isActive(); Just to confirm: Is it guaranteed that m_controller->isActive() returns false in a finite time period? https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/streams/UnderlyingSourceBase.h (right): https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou... third_party/WebKit/Source/core/streams/UnderlyingSourceBase.h:38: UnderlyingSourceBase(ScriptState* scriptState) Add explicit. https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou... third_party/WebKit/Source/core/streams/UnderlyingSourceBase.h:41: this->suspendIfNeeded(); Nit: We normally use a create function. UnderlyingSourceBase* create(...) { UnderlyingSourceBase* underlyingSource = new UnderlyingSourceBase(...); underlyingSource->suspendIfNeeded(); return underlyingSource; } https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou... third_party/WebKit/Source/core/streams/UnderlyingSourceBase.h:44: ReadableStreamController* controller() { return m_controller; } Add const.
https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ReadableStreamController.h (right): https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ReadableStreamController.h:44: if (m_stream.isEmpty()) m_stream can become empty whenever GC runs, so I prefer the following pattern than the current one: v8::Local<v8::Value> stream = m_stream.newLocal(isolate); if (stream.IsEmpty()) return; // Use |stream| below. https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperationsTest.cpp (right): https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperationsTest.cpp:116: class UnderlyingSourceTest final : public UnderlyingSourceBase { Can you rename this class to something like FakeUnderlyingSource, TestUnderlyingSource, etc? UnderlyingSourceTest sounds like inheriting ::testing::Test (like ReadableStreamOperationsTest). https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperationsTest.cpp:118: UnderlyingSourceTest(ScriptState* scriptState) +explicit https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/streams/UnderlyingSourceBase.h (right): https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou... third_party/WebKit/Source/core/streams/UnderlyingSourceBase.h:8: #include "bindings/core/v8/ScriptPromise.h" +ScriptState.h
Sorry that I didn't get to work on this last week.
I moved ReadableStreamController.h back into core/streams per the new policy,
since it is a new file. I did not move ReadableStreamOperations.{h,cpp}; I
figure we should do that in a follow-up commit to make it easier to track
renames.
Biggest outstanding question is about whether these V8 extras functions are
"internal".
https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ReadableStreamController.h
(right):
https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ReadableStreamController.h:44: if
(m_stream.isEmpty())
On 2016/01/25 at 11:10:32, yhirano wrote:
> m_stream can become empty whenever GC runs, so I prefer the following pattern
than the current one:
>
> v8::Local<v8::Value> stream = m_stream.newLocal(isolate);
> if (stream.IsEmpty())
> return;
> // Use |stream| below.
Because GC happens on another thread and can interleave within the lines of the
method? Done.
https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ReadableStreamController.h:51:
v8CallExtraOrCrash(m_scriptState, "CloseReadableStream", args);
On 2016/01/25 at 00:13:29, haraken wrote:
> I haven't yet checked if we should add a MicroTaskSuppressionScope. If this is
an internal function (i.e., not user script), we need to add a
MicroTaskSuppressionScope.
I am not sure what you mean exactly. This is a V8 extra function, so it is kind
of in between. I only added MicrotaskSuppressionScope where not adding it caused
crashes. But maybe we should just add it to all calls into V8 extra functions?
https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ReadableStreamController.h:105:
ScriptState* m_scriptState;
On 2016/01/25 at 00:13:29, haraken wrote:
> This should be RefPtr<ScriptState>. Otherwise it can go stale.
Done. This makes the code more awkward in a number of places... why is there no
implicit conversion from RefPtr<T> to T*?
https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.cpp
(right):
https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.cpp:23:
V8RecursionScope::MicrotaskSuppression mtsScope(scriptState->isolate());
On 2016/01/25 at 00:13:29, haraken wrote:
> Maybe it would make sense to create another version of v8CallExtraOrCrash for
calling an internal function.
>
> - callV8ExtraFunction()
> - callInternalV8ExtraFunction()
Aren't all V8 extra functions internal functions? At least that was my
intent---things you put on the V8 extras binding object should be browser code.
Maybe v8CallExtraOrCrash should use V8ScriptRunner::callInternalFunction instead
of callFunction?
https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperationsTest.cpp
(right):
https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperationsTest.cpp:116:
class UnderlyingSourceTest final : public UnderlyingSourceBase {
On 2016/01/25 at 11:10:32, yhirano wrote:
> Can you rename this class to something like FakeUnderlyingSource,
TestUnderlyingSource, etc?
>
> UnderlyingSourceTest sounds like inheriting ::testing::Test (like
ReadableStreamOperationsTest).
done
https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperationsTest.cpp:118:
UnderlyingSourceTest(ScriptState* scriptState)
On 2016/01/25 at 11:10:33, yhirano wrote:
> +explicit
done
https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/streams/UnderlyingSourceBase.cpp (right):
https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/streams/UnderlyingSourceBase.cpp:51: return
m_controller && m_controller->isActive();
On 2016/01/25 at 00:13:29, haraken wrote:
> Just to confirm: Is it guaranteed that m_controller->isActive() returns false
in a finite time period?
Yes, as long as browser code is well behaved. I have added a comment explaining.
https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/streams/UnderlyingSourceBase.h (right):
https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/streams/UnderlyingSourceBase.h:8: #include
"bindings/core/v8/ScriptPromise.h"
On 2016/01/25 at 11:10:33, yhirano wrote:
> +ScriptState.h
done
https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/streams/UnderlyingSourceBase.h:38:
UnderlyingSourceBase(ScriptState* scriptState)
On 2016/01/25 at 00:13:29, haraken wrote:
> Add explicit.
done
https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/streams/UnderlyingSourceBase.h:41:
this->suspendIfNeeded();
On 2016/01/25 at 00:13:29, haraken wrote:
> Nit: We normally use a create function.
>
> UnderlyingSourceBase* create(...) {
> UnderlyingSourceBase* underlyingSource = new UnderlyingSourceBase(...);
> underlyingSource->suspendIfNeeded();
> return underlyingSource;
> }
I cannot figure out how to do this without making derived classes repeat this
code every time (with changed type names), which seems very bad.
https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/streams/UnderlyingSourceBase.h:44:
ReadableStreamController* controller() { return m_controller; }
On 2016/01/25 at 00:13:29, haraken wrote:
> Add const.
done
https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ReadableStreamController.h (right): https://codereview.chromium.org/1167343002/diff/290001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ReadableStreamController.h:44: if (m_stream.isEmpty()) > Because GC happens on another thread and can interleave within the lines of the > method? No, v8 GC doesn't run unless we use v8 functionality. But in some cases it is easy to overlook v8 calls, so I generally like binding to a v8::Local handle in front. https://codereview.chromium.org/1167343002/diff/310001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.h (right): https://codereview.chromium.org/1167343002/diff/310001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.h:26: static ScriptValue createReadableStream(ScriptState*, UnderlyingSourceBase*, ScriptValue strategy); This operation can throw when passing a certain strategy, right? https://codereview.chromium.org/1167343002/diff/310001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/streams/UnderlyingSourceBase.cpp (right): https://codereview.chromium.org/1167343002/diff/310001/third_party/WebKit/Sou... third_party/WebKit/Source/core/streams/UnderlyingSourceBase.cpp:50: { if (executionContext()->activeDOMObjectsAreStopped()) return false;
https://codereview.chromium.org/1167343002/diff/310001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.h (right): https://codereview.chromium.org/1167343002/diff/310001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.h:26: static ScriptValue createReadableStream(ScriptState*, UnderlyingSourceBase*, ScriptValue strategy); On 2016/02/03 at 11:04:42, yhirano wrote: > This operation can throw when passing a certain strategy, right? Yes, but C++ code should never supply invalid strategies. That is why we use v8CallExtraOrCrash in the implementation: if a bad strategy is supplied from C++ we will crash. https://codereview.chromium.org/1167343002/diff/310001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/streams/UnderlyingSourceBase.cpp (right): https://codereview.chromium.org/1167343002/diff/310001/third_party/WebKit/Sou... third_party/WebKit/Source/core/streams/UnderlyingSourceBase.cpp:50: { On 2016/02/03 at 11:04:43, yhirano wrote: > if (executionContext()->activeDOMObjectsAreStopped()) > return false; added
lgtm, thanks! https://codereview.chromium.org/1167343002/diff/310001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.h (right): https://codereview.chromium.org/1167343002/diff/310001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.h:26: static ScriptValue createReadableStream(ScriptState*, UnderlyingSourceBase*, ScriptValue strategy); On 2016/02/03 21:20:16, domenic wrote: > On 2016/02/03 at 11:04:42, yhirano wrote: > > This operation can throw when passing a certain strategy, right? > > Yes, but C++ code should never supply invalid strategies. That is why we use > v8CallExtraOrCrash in the implementation: if a bad strategy is supplied from C++ > we will crash. I see. Please write so as a comment here.
https://codereview.chromium.org/1167343002/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.cpp (right): https://codereview.chromium.org/1167343002/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.cpp:5: #include "bindings/core/v8/ReadableStreamOperations.h" Per the discussion in blink-dev@, feel free to move this file to core/streams/ if you think it would make more sense (in a follow-up CL). https://codereview.chromium.org/1167343002/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.cpp:25: v8::Local<v8::Value> jsStream = v8CallExtraOrCrash(scriptState, "createReadableStreamWithExternalController", args); Sorry for asking you many things around this but would you move v8CallExtraOrCrash into V8ScriptRunner? We want to encapsulate all the complexity of invoking JS code into V8ScriptRunner. V8RecursionScope::MicrotaskSuppression should be moved into the method of V8ScriptRunner. https://codereview.chromium.org/1167343002/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperationsTest.cpp (right): https://codereview.chromium.org/1167343002/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperationsTest.cpp:124: void enqueue(ScriptValue v) { controller()->enqueue(v); } v => value https://codereview.chromium.org/1167343002/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperationsTest.cpp:126: void error(ScriptValue e) { controller()->error(e); } e => value https://codereview.chromium.org/1167343002/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/streams/ReadableStreamController.h (right): https://codereview.chromium.org/1167343002/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/core/streams/ReadableStreamController.h:44: { You need to add: if (!m_scriptState->contextIsValid()) return; before using the ScriptState. The same comment for other places where you're using ScriptStates. https://codereview.chromium.org/1167343002/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/core/streams/ReadableStreamController.h:54: v8CallExtraOrCrash(scriptState, "CloseReadableStream", args); Don't you need to add MicroTaskSuppression? https://codereview.chromium.org/1167343002/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/core/streams/ReadableStreamController.h:92: void error(ErrorType e) e => errorType https://codereview.chromium.org/1167343002/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/streams/UnderlyingSourceBase.cpp (right): https://codereview.chromium.org/1167343002/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/core/streams/UnderlyingSourceBase.cpp:51: if (executionContext()->activeDOMObjectsAreStopped()) It looks a bit strange that you need to have the activeDOMObjectsAreStopped() check in hasPendingActivity(). Is it really okay to collect the wrapper even though the execution context can be resumed in the future?
https://codereview.chromium.org/1167343002/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/streams/UnderlyingSourceBase.cpp (right): https://codereview.chromium.org/1167343002/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/core/streams/UnderlyingSourceBase.cpp:51: if (executionContext()->activeDOMObjectsAreStopped()) On 2016/02/04 01:27:50, haraken wrote: > > It looks a bit strange that you need to have the activeDOMObjectsAreStopped() > check in hasPendingActivity(). > > Is it really okay to collect the wrapper even though the execution context can > be resumed in the future? haraken@, are you talking about activeDOMObjectsAreSuspended? IIUC "stop" is irrevocable.
On 2016/02/04 01:31:28, yhirano wrote: > https://codereview.chromium.org/1167343002/diff/330001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/streams/UnderlyingSourceBase.cpp (right): > > https://codereview.chromium.org/1167343002/diff/330001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/streams/UnderlyingSourceBase.cpp:51: if > (executionContext()->activeDOMObjectsAreStopped()) > On 2016/02/04 01:27:50, haraken wrote: > > > > It looks a bit strange that you need to have the activeDOMObjectsAreStopped() > > check in hasPendingActivity(). > > > > Is it really okay to collect the wrapper even though the execution context can > > be resumed in the future? > > haraken@, are you talking about activeDOMObjectsAreSuspended? IIUC "stop" is > irrevocable. Ah, you're right. Instead of adding the activeDOMObjectsAreStopped() check here, it would be better to override stop() and clear m_controller in stop().
On 2016/02/04 at 01:27:50, haraken wrote: > https://codereview.chromium.org/1167343002/diff/330001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/streams/ReadableStreamController.h (right): > > https://codereview.chromium.org/1167343002/diff/330001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/streams/ReadableStreamController.h:44: { > > You need to add: > > if (!m_scriptState->contextIsValid()) > return; > > before using the ScriptState. The same comment for other places where you're using ScriptStates. For my education, why is this? That is, when would contextIsValid() be false? I haven't seen this pattern before in the (limited) Blink code I've explored so it is a bit confusing to me. In fact it seems like most code just relies on the ASSERT inside ScriptState::Scope to cause a crash when this is false. Is it because the ActiveDOMObject could outlive the context? > > https://codereview.chromium.org/1167343002/diff/330001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/streams/ReadableStreamController.h:54: v8CallExtraOrCrash(scriptState, "CloseReadableStream", args); > > Don't you need to add MicroTaskSuppression? > > https://codereview.chromium.org/1167343002/diff/330001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.cpp:25: v8::Local<v8::Value> jsStream = v8CallExtraOrCrash(scriptState, "createReadableStreamWithExternalController", args); > > Sorry for asking you many things around this but would you move v8CallExtraOrCrash into V8ScriptRunner? We want to encapsulate all the complexity of invoking JS code into V8ScriptRunner. V8RecursionScope::MicrotaskSuppression should be moved into the method of V8ScriptRunner. > I asked some related questions up-thread in my previous replies. So, let's check my understanding. Is this a good plan? - Move v8CallExtraOrCrash (and friends) into V8ScriptRunner. - Change them to use V8ScriptRunner::callInternalFunction instead of V8ScriptRunner::callFunction. This will automatically do the microtask suppression for us and will avoid misleading the inspector into thinking author code is called. I guess I will do that in a separate CL and rebase this one on top of it.
On 2016/02/04 02:46:58, domenic wrote: > On 2016/02/04 at 01:27:50, haraken wrote: > > > > https://codereview.chromium.org/1167343002/diff/330001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/streams/ReadableStreamController.h > (right): > > > > > https://codereview.chromium.org/1167343002/diff/330001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/streams/ReadableStreamController.h:44: { > > > > You need to add: > > > > if (!m_scriptState->contextIsValid()) > > return; > > > > before using the ScriptState. The same comment for other places where you're > using ScriptStates. > > For my education, why is this? That is, when would contextIsValid() be false? > > I haven't seen this pattern before in the (limited) Blink code I've explored so > it is a bit confusing to me. In fact it seems like most code just relies on the > ASSERT inside ScriptState::Scope to cause a crash when this is false. Is it > because the ActiveDOMObject could outlive the context? You need to check scriptState->contextIsValid() when you use a ScriptState except the current ScriptState. A common programming pattern is: class X { X(ScriptState* scriptState) : m_scriptState(scriptState) { } // Store the current ScriptState when X is constructed. void fired() { if (m_scriptState->contextIsValid()) // The context may be gone when the fired() is fired asynchronously. return; ScriptState::Scope scope(m_scriptState.get()); ...; } RefPtr<ScriptState> m_scriptState; }; > > > > > > https://codereview.chromium.org/1167343002/diff/330001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/streams/ReadableStreamController.h:54: > v8CallExtraOrCrash(scriptState, "CloseReadableStream", args); > > > > Don't you need to add MicroTaskSuppression? > > > > > https://codereview.chromium.org/1167343002/diff/330001/third_party/WebKit/Sou... > > third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.cpp:25: > v8::Local<v8::Value> jsStream = v8CallExtraOrCrash(scriptState, > "createReadableStreamWithExternalController", args); > > > > Sorry for asking you many things around this but would you move > v8CallExtraOrCrash into V8ScriptRunner? We want to encapsulate all the > complexity of invoking JS code into V8ScriptRunner. > V8RecursionScope::MicrotaskSuppression should be moved into the method of > V8ScriptRunner. > > > > I asked some related questions up-thread in my previous replies. So, let's check > my understanding. Is this a good plan? > > - Move v8CallExtraOrCrash (and friends) into V8ScriptRunner. > - Change them to use V8ScriptRunner::callInternalFunction instead of > V8ScriptRunner::callFunction. This will automatically do the microtask > suppression for us and will avoid misleading the inspector into thinking author > code is called. > > I guess I will do that in a separate CL and rebase this one on top of it. Sounds like a good plan!
https://codereview.chromium.org/1167343002/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/streams/ReadableStreamController.h (right): https://codereview.chromium.org/1167343002/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/core/streams/ReadableStreamController.h:44: { On 2016/02/04 01:27:50, haraken wrote: > > You need to add: > > if (!m_scriptState->contextIsValid()) > return; > > before using the ScriptState. The same comment for other places where you're > using ScriptStates. Sorry for my ignorance, is it needed even when we're sure that the associated ExecutionContext is not stopped?
https://codereview.chromium.org/1167343002/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/streams/ReadableStreamController.h (right): https://codereview.chromium.org/1167343002/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/core/streams/ReadableStreamController.h:44: { On 2016/02/05 02:15:42, yhirano wrote: > On 2016/02/04 01:27:50, haraken wrote: > > > > You need to add: > > > > if (!m_scriptState->contextIsValid()) > > return; > > > > before using the ScriptState. The same comment for other places where you're > > using ScriptStates. > > Sorry for my ignorance, is it needed even when we're sure that the associated > ExecutionContext is not stopped? Ah, then not needed. Then shall we add ASSERT(scriptState->contextIsValid())?
On 2016/02/05 at 02:32:27, haraken wrote: > Ah, then not needed. Then shall we add ASSERT(scriptState->contextIsValid())? This is automatically done by ScriptState::Scope(scriptState). Let's see if I can chromote into my work machine and get this CL updated before Tokyo end of day...
On 2016/02/05 at 03:48:30, domenic wrote: > Let's see if I can chromote into my work machine and get this CL updated before Tokyo end of day... OK, should be ready for another round of review. Rebased on https://codereview.chromium.org/1670743002.
LGTM with one question. https://codereview.chromium.org/1167343002/diff/390001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/streams/ReadableStreamController.h (right): https://codereview.chromium.org/1167343002/diff/390001/third_party/WebKit/Sou... third_party/WebKit/Source/core/streams/ReadableStreamController.h:61: ScriptState* scriptState = m_scriptState.get(); How is it guaranteed that scriptState->contextIsValid is true (i.e., the execution context has not yet stopped) when the desiredSize is called?
On 2016/02/05 at 04:56:54, haraken wrote: > LGTM with one question. > > https://codereview.chromium.org/1167343002/diff/390001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/streams/ReadableStreamController.h (right): > > https://codereview.chromium.org/1167343002/diff/390001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/streams/ReadableStreamController.h:61: ScriptState* scriptState = m_scriptState.get(); > > How is it guaranteed that scriptState->contextIsValid is true (i.e., the execution context has not yet stopped) when the desiredSize is called? yhirano@ may have a better answer, but I think it would be buggy browser code if it tries to manipulate a stream (through its controller) when the execution context for that stream has stopped. I am not sure though, since I am not very familiar with how Blink generally handles these kind of resources. Maybe the better practice is to just allow such use, because it is hard to control and audit all call sites, and then force them to no-op? In which case we should add the `if(...) return` back. Happy to do whichever you two decide but I don't think I have enough Blink experience to know the right answer myself.
https://codereview.chromium.org/1167343002/diff/390001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/streams/ReadableStreamController.h (right): https://codereview.chromium.org/1167343002/diff/390001/third_party/WebKit/Sou... third_party/WebKit/Source/core/streams/ReadableStreamController.h:61: ScriptState* scriptState = m_scriptState.get(); On 2016/02/05 04:56:54, haraken wrote: > > How is it guaranteed that scriptState->contextIsValid is true (i.e., the > execution context has not yet stopped) when the desiredSize is called? If it's not obvious, I'd prefer just inserting: if (!m_scriptState->contextIsValid()) return 0; But let's wait for yhirano's thoughts.
https://codereview.chromium.org/1167343002/diff/390001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/streams/ReadableStreamController.h (right): https://codereview.chromium.org/1167343002/diff/390001/third_party/WebKit/Sou... third_party/WebKit/Source/core/streams/ReadableStreamController.h:61: ScriptState* scriptState = m_scriptState.get(); On 2016/02/05 05:30:08, haraken wrote: > On 2016/02/05 04:56:54, haraken wrote: > > > > How is it guaranteed that scriptState->contextIsValid is true (i.e., the > > execution context has not yet stopped) when the desiredSize is called? > > If it's not obvious, I'd prefer just inserting: > > if (!m_scriptState->contextIsValid()) > return 0; > > But let's wait for yhirano's thoughts. Inserting early return statements is OK. I'm also OK with just writing comments so that close / desiredSize / enqueue / error must not be called when the ScriptState is invalid. I agree with domenic's following comment > I think it would be buggy browser code if it tries to manipulate a stream (through its controller) when the execution context for that stream has stopped.
Regarding MicrotaskSuppression, let me check my understanding: V8RecursionScope calls Microtask::performCheckpoint, corresponding to "perform a microtask checkpoint" in the HTML spec (https://html.spec.whatwg.org/multipage/webappapis.html#perform-a-microtask-ch...), when exiting from the outermost scope. If a (blink) developer calls a JS function but does not want to perform a microtask checkpoint, he/she must place a MicrotaskSuppression. MicrotaskSuppression is for checking and does nothing on the release mode. Every time a JS function is called, assertV8RecursionScope will be called to check if the developer forgets to place a V8RecursionScope or a MicrotaskSuppression. I don't know the reason, but Promise related calls are exempted because they use PREPARE_FOR_EXECUTION in v8/src/api.cc rather than PREPARE_FOR_EXECUTION_WITH_CALLBACK and friends used by CallAsFunction. The former doesn't call assertV8RecursionScope via the "CallCompletedCallback" v8 mechanism because CallDepthScope is constructed without do_callback set in the macro.
On 2016/02/05 08:00:54, yhirano wrote: > Regarding MicrotaskSuppression, let me check my understanding: > > V8RecursionScope calls Microtask::performCheckpoint, corresponding to "perform a > microtask checkpoint" in the HTML spec > (https://html.spec.whatwg.org/multipage/webappapis.html#perform-a-microtask-ch...), > when exiting from the outermost scope. If a (blink) developer calls a JS > function but does not want to perform a microtask checkpoint, he/she must place > a MicrotaskSuppression. MicrotaskSuppression is for checking and does nothing on > the release mode. Every time a JS function is called, assertV8RecursionScope > will be called to check if the developer forgets to place a V8RecursionScope or > a MicrotaskSuppression. > > I don't know the reason, but Promise related calls are exempted because they use > PREPARE_FOR_EXECUTION in v8/src/api.cc rather than > PREPARE_FOR_EXECUTION_WITH_CALLBACK and friends used by CallAsFunction. The > former doesn't call assertV8RecursionScope via the "CallCompletedCallback" v8 > mechanism because CallDepthScope is constructed without do_callback set in the > macro. If the understanding is correct, I *think* we need MicrotaskSuppression in close / error / desiredSize / enqueue because - We don't want to perform a microtask checkpoint, and - We usually don't want the caller to place a MicrotaskSuppression or a V8RecurtionScope. Microtasks enqueued in the calls will run at the end of the blink task if no one places a V8RecursionScope on the call site. Please correct me if I'm wrong.
On 2016/02/05 08:12:40, yhirano wrote: > On 2016/02/05 08:00:54, yhirano wrote: > > Regarding MicrotaskSuppression, let me check my understanding: > > > > V8RecursionScope calls Microtask::performCheckpoint, corresponding to "perform > a > > microtask checkpoint" in the HTML spec > > > (https://html.spec.whatwg.org/multipage/webappapis.html#perform-a-microtask-ch...), > > when exiting from the outermost scope. If a (blink) developer calls a JS > > function but does not want to perform a microtask checkpoint, he/she must > place > > a MicrotaskSuppression. MicrotaskSuppression is for checking and does nothing > on > > the release mode. Every time a JS function is called, assertV8RecursionScope > > will be called to check if the developer forgets to place a V8RecursionScope > or > > a MicrotaskSuppression. > > > > I don't know the reason, but Promise related calls are exempted because they > use > > PREPARE_FOR_EXECUTION in v8/src/api.cc rather than > > PREPARE_FOR_EXECUTION_WITH_CALLBACK and friends used by CallAsFunction. The > > former doesn't call assertV8RecursionScope via the "CallCompletedCallback" v8 > > mechanism because CallDepthScope is constructed without do_callback set in the > > macro. > > If the understanding is correct, I *think* we need MicrotaskSuppression in close > / error / desiredSize / enqueue because > - We don't want to perform a microtask checkpoint, and > - We usually don't want the caller to place a MicrotaskSuppression or a > V8RecurtionScope. > > Microtasks enqueued in the calls will run at the end of the blink task if no one > places a V8RecursionScope on the call site. > > Please correct me if I'm wrong. Yes, you're correct. We should have a MicrotaskSuppression, and we do :) callExtraOrCrash calls V8ScriptRunner::callInternalFunction (see this CL: https://codereview.chromium.org/1670743002/), which puts a MicrotaskSuppression.
On 2016/02/05 09:38:45, haraken wrote: > On 2016/02/05 08:12:40, yhirano wrote: > > On 2016/02/05 08:00:54, yhirano wrote: > > > Regarding MicrotaskSuppression, let me check my understanding: > > > > > > V8RecursionScope calls Microtask::performCheckpoint, corresponding to > "perform > > a > > > microtask checkpoint" in the HTML spec > > > > > > (https://html.spec.whatwg.org/multipage/webappapis.html#perform-a-microtask-ch...), > > > when exiting from the outermost scope. If a (blink) developer calls a JS > > > function but does not want to perform a microtask checkpoint, he/she must > > place > > > a MicrotaskSuppression. MicrotaskSuppression is for checking and does > nothing > > on > > > the release mode. Every time a JS function is called, assertV8RecursionScope > > > will be called to check if the developer forgets to place a V8RecursionScope > > or > > > a MicrotaskSuppression. > > > > > > I don't know the reason, but Promise related calls are exempted because they > > use > > > PREPARE_FOR_EXECUTION in v8/src/api.cc rather than > > > PREPARE_FOR_EXECUTION_WITH_CALLBACK and friends used by CallAsFunction. The > > > former doesn't call assertV8RecursionScope via the "CallCompletedCallback" > v8 > > > mechanism because CallDepthScope is constructed without do_callback set in > the > > > macro. > > > > If the understanding is correct, I *think* we need MicrotaskSuppression in > close > > / error / desiredSize / enqueue because > > - We don't want to perform a microtask checkpoint, and > > - We usually don't want the caller to place a MicrotaskSuppression or a > > V8RecurtionScope. > > > > Microtasks enqueued in the calls will run at the end of the blink task if no > one > > places a V8RecursionScope on the call site. > > > > Please correct me if I'm wrong. > > Yes, you're correct. We should have a MicrotaskSuppression, and we do :) > > callExtraOrCrash calls V8ScriptRunner::callInternalFunction (see this CL: > https://codereview.chromium.org/1670743002/), which puts a MicrotaskSuppression. Ah, I see, thank you!
I am going to add comments that ScriptState needs to be valid, and then commit. If it turns out that early-return is better behavior I imagine we will discover that when the ASSERTs start happening :). But for now I think we should assume any code that calls these when the execution context is dead is buggy code.
The CQ bit was checked by domenic@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1167343002/#ps410001 (title: "Add comments saying context must be valid")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1167343002/410001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1167343002/410001
The CQ bit was unchecked by commit-bot@chromium.org
CLs for remote refs other than refs/pending/heads/master must contain NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them
The CQ bit was checked by domenic@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/1167343002/#ps430001 (title: "rebase on master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1167343002/430001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1167343002/430001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by domenic@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/1167343002/#ps450001 (title: "Fix error that clang catches but MSVS does not")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1167343002/450001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1167343002/450001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2016/02/05 at 20:26:15, commit-bot wrote: > Try jobs failed on following builders: > linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) > mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) It looks like there is some include cycle between ToV8.h and ScriptValue.h that my CL exposes, but which only occurs on Linux so I can't debug it locally. On Monday I will attempt to find a Linux box to SSH into, or use my laptop, or try clang on Windows. In the meantime if you guys have any idea on the best fix I'd love to hear it. My guess is I'll just have to sprinkle #include calls in a few places manually, which is pretty unsatisfying.
The CQ bit was checked by domenic@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1167343002/470001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1167343002/470001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by domenic@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1167343002/490001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1167343002/490001
My latest patch set seems to fix the compile error by moving the implementation of ScriptState::from to ToV8.h. I am not sure that is the best solution so I'd like a review of it from haraken@ before committing. See ToV8.h and ScriptValue.h.
https://codereview.chromium.org/1167343002/diff/490001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ToV8.h (right): https://codereview.chromium.org/1167343002/diff/490001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ToV8.h:189: inline v8::Local<v8::Value> toV8(const ScriptValue& value, v8::Local<v8::Object> creationContext, v8::Isolate*) Alternately, can we move this implementation to ToV8.cpp and remove '#include ScriptValue.h' from ToV8.h?
On 2016/02/09 at 01:19:30, haraken wrote: > https://codereview.chromium.org/1167343002/diff/490001/third_party/WebKit/Sou... > File third_party/WebKit/Source/bindings/core/v8/ToV8.h (right): > > https://codereview.chromium.org/1167343002/diff/490001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/ToV8.h:189: inline v8::Local<v8::Value> toV8(const ScriptValue& value, v8::Local<v8::Object> creationContext, v8::Isolate*) > > Alternately, can we move this implementation to ToV8.cpp and remove '#include ScriptValue.h' from ToV8.h? I just tried that, but it generates a whole new mess of errors, e.g. In file included from ..\..\third_party\WebKit\Source\core\html\FormDataTest.cpp:5: In file included from ..\..\third_party\WebKit\Source\core/html/FormData.h:34: In file included from ..\..\third_party\WebKit\Source\bindings/core/v8/Iterable.h:8: In file included from ..\..\third_party\WebKit\Source\bindings/core/v8/V8IteratorResultValue.h:8: In file included from ..\..\third_party\WebKit\Source\bindings/core/v8/ScriptValue.h:37: In file included from ..\..\third_party\WebKit\Source\bindings/core/v8/ToV8.h:14: ..\..\third_party\WebKit\Source\bindings/core/v8/V8Binding.h(938,16) : error: invalid use of incomplete type 'blink::ScriptValue' return ScriptValue(ScriptState::current(isolate), value); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ..\..\third_party\WebKit\Source\bindings/core/v8/ScriptState.h(22,7) : note: forward declaration of 'blink::ScriptValue' class ScriptValue; I tried to start fixing that by modifying ScriptState.h to include ScriptValue.h instead of using a forward declaration, but that went even more wrong. In general I think the dependency graph between all these files (ScriptState.h, ScriptValue.h, ToV8.h, V8Binding.h, and their dependents) is very complicated and it would be a good follow-up task for someone (not me :) to straighten it out. I'd prefer to get my CL landed without having to do that myself, especially since my Windows box can't debug these issues easily. I'm happy to try any other suggestions you have though.
On 2016/02/09 01:38:26, domenic wrote: > On 2016/02/09 at 01:19:30, haraken wrote: > > > https://codereview.chromium.org/1167343002/diff/490001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/bindings/core/v8/ToV8.h (right): > > > > > https://codereview.chromium.org/1167343002/diff/490001/third_party/WebKit/Sou... > > third_party/WebKit/Source/bindings/core/v8/ToV8.h:189: inline > v8::Local<v8::Value> toV8(const ScriptValue& value, v8::Local<v8::Object> > creationContext, v8::Isolate*) > > > > Alternately, can we move this implementation to ToV8.cpp and remove '#include > ScriptValue.h' from ToV8.h? > > I just tried that, but it generates a whole new mess of errors, e.g. > > In file included from > ..\..\third_party\WebKit\Source\core\html\FormDataTest.cpp:5: > In file included from ..\..\third_party\WebKit\Source\core/html/FormData.h:34: > In file included from > ..\..\third_party\WebKit\Source\bindings/core/v8/Iterable.h:8: > In file included from > ..\..\third_party\WebKit\Source\bindings/core/v8/V8IteratorResultValue.h:8: > In file included from > ..\..\third_party\WebKit\Source\bindings/core/v8/ScriptValue.h:37: > In file included from > ..\..\third_party\WebKit\Source\bindings/core/v8/ToV8.h:14: > ..\..\third_party\WebKit\Source\bindings/core/v8/V8Binding.h(938,16) : error: > invalid use of incomplete type 'blink::ScriptValue' > return ScriptValue(ScriptState::current(isolate), value); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ..\..\third_party\WebKit\Source\bindings/core/v8/ScriptState.h(22,7) : note: > forward declaration of 'blink::ScriptValue' > class ScriptValue; > > I tried to start fixing that by modifying ScriptState.h to include ScriptValue.h > instead of using a forward declaration, but that went even more wrong. > > In general I think the dependency graph between all these files (ScriptState.h, > ScriptValue.h, ToV8.h, V8Binding.h, and their dependents) is very complicated > and it would be a good follow-up task for someone (not me :) to straighten it > out. I'd prefer to get my CL landed without having to do that myself, especially > since my Windows box can't debug these issues easily. I'm happy to try any other > suggestions you have though. OK, LGTM :)
Description was changed from
==========
Adds
ReadableStreamOperations::{createReadableStream,createCountQueuingStrategy}, as
well as the supporting UnderlyingSourceBase and ReadableStreamController
classes. The test demonstrates how to use these.
R=yhirano@chromium.org,haraken@chromium.org
BUG=503491
==========
to
==========
Adds
ReadableStreamOperations::{createReadableStream,createCountQueuingStrategy}, as
well as the supporting UnderlyingSourceBase and ReadableStreamController
classes. The test demonstrates how to use these.
This revealed a circular dependency between ToV8.h and ScriptValue.h that had to
be resolved by moving one of the ScriptValue::from overloads into ToV8.h. While
doing so, convert it to use perfect forwarding.
R=yhirano@chromium.org,haraken@chromium.org
BUG=503491
==========
The CQ bit was unchecked by domenic@chromium.org
The CQ bit was checked by domenic@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/1167343002/#ps490001 (title: "Trying blindly to resolve circular dependency")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1167343002/490001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1167343002/490001
Message was sent while issue was closed.
Description was changed from
==========
Adds
ReadableStreamOperations::{createReadableStream,createCountQueuingStrategy}, as
well as the supporting UnderlyingSourceBase and ReadableStreamController
classes. The test demonstrates how to use these.
This revealed a circular dependency between ToV8.h and ScriptValue.h that had to
be resolved by moving one of the ScriptValue::from overloads into ToV8.h. While
doing so, convert it to use perfect forwarding.
R=yhirano@chromium.org,haraken@chromium.org
BUG=503491
==========
to
==========
Adds
ReadableStreamOperations::{createReadableStream,createCountQueuingStrategy}, as
well as the supporting UnderlyingSourceBase and ReadableStreamController
classes. The test demonstrates how to use these.
This revealed a circular dependency between ToV8.h and ScriptValue.h that had to
be resolved by moving one of the ScriptValue::from overloads into ToV8.h. While
doing so, convert it to use perfect forwarding.
R=yhirano@chromium.org,haraken@chromium.org
BUG=503491
==========
Message was sent while issue was closed.
Committed patchset #26 (id:490001)
Message was sent while issue was closed.
Description was changed from
==========
Adds
ReadableStreamOperations::{createReadableStream,createCountQueuingStrategy}, as
well as the supporting UnderlyingSourceBase and ReadableStreamController
classes. The test demonstrates how to use these.
This revealed a circular dependency between ToV8.h and ScriptValue.h that had to
be resolved by moving one of the ScriptValue::from overloads into ToV8.h. While
doing so, convert it to use perfect forwarding.
R=yhirano@chromium.org,haraken@chromium.org
BUG=503491
==========
to
==========
Adds
ReadableStreamOperations::{createReadableStream,createCountQueuingStrategy}, as
well as the supporting UnderlyingSourceBase and ReadableStreamController
classes. The test demonstrates how to use these.
This revealed a circular dependency between ToV8.h and ScriptValue.h that had to
be resolved by moving one of the ScriptValue::from overloads into ToV8.h. While
doing so, convert it to use perfect forwarding.
R=yhirano@chromium.org,haraken@chromium.org
BUG=503491
Committed: https://crrev.com/81c4571248f30446f95901a76a5059929b467434
Cr-Commit-Position: refs/heads/master@{#374308}
==========
Message was sent while issue was closed.
Patchset 26 (id:??) landed as https://crrev.com/81c4571248f30446f95901a76a5059929b467434 Cr-Commit-Position: refs/heads/master@{#374308} |
