Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1539803002:
[Fetch API] Fix a memory leak with a Response constructed with a ReadableStream (Closed)
|
Created:
5 years ago by yhirano Modified:
4 years, 10 months ago CC:
blink-reviews, blink-reviews-bindings_chromium.org, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@response-constructed-with-stream Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Fetch API] Fix a memory leak with a Response constructed with a ReadableStream
This CL fixes a memory leak with a Response constructed with a ReadableStream
by holding a weak persistent in ReadableStreamDataConsumerHandle.
The stream reader will be kept alive by the hidden reference in Response.
BUG=564479
Committed: https://crrev.com/f1bb6e7acafff4116f8f6100f9612c93930c2ecc
Cr-Commit-Position: refs/heads/master@{#375196}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 18
Patch Set 4 : #
Total comments: 14
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Messages
Total messages: 29 (8 generated)
Description was changed from ========== [Fetch API] Fix a memory leak with a Response constructed with a ReadableStream BUG=564479 ========== to ========== [Fetch API] Fix a memory leak with a Response constructed with a ReadableStream This CL fixes a memory leak with a Response constructed with a ReadableStream by holding a weak persistent in ReadableStreamDataConsumerHandle. The stream reader will be kept alive by the hidden reference in Response. BUG=564479 ==========
yhirano@chromium.org changed reviewers: + bashi@google.com, tyoshino@chromium.org
ping
bashi@chromium.org changed reviewers: + bashi@chromium.org
lgtm https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/Response.cpp:144: bodyHandle = ReadableStreamDataConsumerHandle::create(scriptState, reader.v8Value().As<v8::Object>()); just to clarify: reader.v8Value() is always an Object?
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8HiddenValue.h (right): https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8HiddenValue.h:43: V(readableStreamReaderInResponse) \ alphabetical order. https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp (right): https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp:274: data.GetParameter()->onCollected(); ReadableStreamDataConsumerHandle.cpp uses a bunch of V8 APIs. We want to limit direct V8 API usage only to bindings/, so consider moving (a part of) ReadableStreamDataConsumerHandle.cpp to bindings/. You don't need to address it in this CL though. https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp:281: ScopedPersistent<v8::Object> m_reader; Instead of holding a weak reference, how about holding a pointer to the Response object and retrieve the V8 object from the Response object's wrapper? https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp:282: ScriptState* m_scriptState; This must be RefPtr<ScriptState>. https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/Response.cpp:158: // the Response is valid. the the https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/Response.cpp:160: ASSERT(!wrapper.IsEmpty()); toV8 can return an empty handle.
https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8HiddenValue.h (right): https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8HiddenValue.h:43: V(readableStreamReaderInResponse) \ On 2016/01/06 05:49:06, haraken wrote: > > alphabetical order. Done. https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp (right): https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp:274: data.GetParameter()->onCollected(); On 2016/01/06 05:49:06, haraken wrote: > > ReadableStreamDataConsumerHandle.cpp uses a bunch of V8 APIs. We want to limit > direct V8 API usage only to bindings/, so consider moving (a part of) > ReadableStreamDataConsumerHandle.cpp to bindings/. You don't need to address it > in this CL though. Is this point affected by the policy change currently discussed? == Let me confirm: 1. Regarding OnFulfilled::call, I can move the code to ReadableStreamOperations.{cpp,h}. 2. Regarding ScopedPersistent, All we need is weak ScriptValue and there are several solutions. a. I can implement makeWeak on ScriptValue and use it in this file. b. I can implement a custom wrapper class for this specific use-case, but I don't like it. c. I have a plan to make this class on-heap. IIRC in the future an on-heap object can hold a ScriptValue, right? Then we will not need weak-related code. Are there other part you want to move to bindings? If so, I think I need to move the entire file to bindings/. https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp:281: ScopedPersistent<v8::Object> m_reader; On 2016/01/06 05:49:06, haraken wrote: > > Instead of holding a weak reference, how about holding a pointer to the Response > object and retrieve the V8 object from the Response object's wrapper? I would keep that a Response is constructed from a WebDataConsumerHandle. That means we cannot have a Response here because it doesn't exist when constructed (and after that we can call only WebDataConsumerHandle methods). https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp:282: ScriptState* m_scriptState; On 2016/01/06 05:49:06, haraken wrote: > > This must be RefPtr<ScriptState>. Done. https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/Response.cpp:144: bodyHandle = ReadableStreamDataConsumerHandle::create(scriptState, reader.v8Value().As<v8::Object>()); On 2016/01/06 05:39:17, bashi1 wrote: > just to clarify: reader.v8Value() is always an Object? Deleted, but yes, ReadableStreamOperations::isReadableStream checks it. https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/Response.cpp:158: // the Response is valid. On 2016/01/06 05:49:06, haraken wrote: > > the the Done. https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/Response.cpp:160: ASSERT(!wrapper.IsEmpty()); On 2016/01/06 05:49:06, haraken wrote: > > toV8 can return an empty handle. Done.
https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp (right): https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp:274: data.GetParameter()->onCollected(); On 2016/01/26 05:09:32, yhirano wrote: > On 2016/01/06 05:49:06, haraken wrote: > > > > ReadableStreamDataConsumerHandle.cpp uses a bunch of V8 APIs. We want to limit > > direct V8 API usage only to bindings/, so consider moving (a part of) > > ReadableStreamDataConsumerHandle.cpp to bindings/. You don't need to address > it > > in this CL though. > > Is this point affected by the policy change currently discussed? > > == > > Let me confirm: > > 1. Regarding OnFulfilled::call, I can move the code to > ReadableStreamOperations.{cpp,h}. > 2. Regarding ScopedPersistent, All we need is weak ScriptValue and there are > several solutions. > a. I can implement makeWeak on ScriptValue and use it in this file. > b. I can implement a custom wrapper class for this specific use-case, but I > don't like it. > c. I have a plan to make this class on-heap. IIRC in the future an on-heap > object can hold a ScriptValue, right? Then we will not need > weak-related code. > > Are there other part you want to move to bindings? If so, I think I need to move > the entire file to bindings/. Yes, I think our new policy is going to allow V8 APIs in core/. I'll start the thread in blink-dev@ today.
ping?
Sorry for the review delay! Mostly looks good. https://codereview.chromium.org/1539803002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp (right): https://codereview.chromium.org/1539803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp:165: ReadableStreamOperations::read(m_scriptState.get(), reader).then( Would you create a helper function in V8ScriptRunner for this one? I want to encapsulate all the complexity about V8RecursionScope, Microtask etc into the helper functions in V8ScriptRunner. https://codereview.chromium.org/1539803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp:264: // outside of ReadableStreamDataConsumerHandle. Just to confirm: Are you assuming that ReadableStreamDataConsumerHandle can be kept alive even after the Reader's wrapper is collected, right? Then it makes sense to use a weak persistent here. https://codereview.chromium.org/1539803002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/1539803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/Response.cpp:143: reader = ScriptValue(); If getReader throws an exception, it would be better to return an empty reader from the getReader; i.e., remove this line. https://codereview.chromium.org/1539803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/Response.cpp:145: exceptionState.clearException(); Is it okay to clear the exception here? Then the caller site won't be able to detect that Response::create has failed at creating a reader. https://codereview.chromium.org/1539803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/Response.cpp:162: v8::Local<v8::Value> wrapper = toV8(response, scriptState->context()->Global(), scriptState->isolate()); Shall we enter ScriptState::Scope before calling toV8? https://codereview.chromium.org/1539803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/Response.cpp:162: v8::Local<v8::Value> wrapper = toV8(response, scriptState->context()->Global(), scriptState->isolate()); Can we use toV8(response, scriptState) ?
https://codereview.chromium.org/1539803002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp (right): https://codereview.chromium.org/1539803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp:165: ReadableStreamOperations::read(m_scriptState.get(), reader).then( On 2016/01/30 15:35:53, haraken wrote: > > Would you create a helper function in V8ScriptRunner for this one? > > I want to encapsulate all the complexity about V8RecursionScope, Microtask etc > into the helper functions in V8ScriptRunner. Do you have an idea how to encapsulate these calls without loss of generality? https://codereview.chromium.org/1539803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp:264: // outside of ReadableStreamDataConsumerHandle. On 2016/01/30 15:35:53, haraken wrote: > > Just to confirm: Are you assuming that ReadableStreamDataConsumerHandle can be > kept alive even after the Reader's wrapper is collected, right? Then it makes > sense to use a weak persistent here. > A typical object graph: Response wrapper -> Response -> BodyStreamBuffer -> ReadableStreamDataConsumerHandle -> |m_reader|. The JS object referenced by |m_reader| can reference the Response wrapper and hence I want to make the last reference link weak. https://codereview.chromium.org/1539803002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/1539803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/Response.cpp:143: reader = ScriptValue(); On 2016/01/30 15:35:53, haraken wrote: > > If getReader throws an exception, it would be better to return an empty reader > from the getReader; i.e., remove this line. At this moment I would like to not throw an exception here, and make the stream errored. A user can detect the error when he / she tries to read data from the stream (i.e. new Response(locked_stream).text() rejects). This is a fetch & streams spec related issue rather than a binding issue, I think. https://codereview.chromium.org/1539803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/Response.cpp:145: exceptionState.clearException(); On 2016/01/30 15:35:53, haraken wrote: > > Is it okay to clear the exception here? Then the caller site won't be able to > detect that Response::create has failed at creating a reader. Ditto https://codereview.chromium.org/1539803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/Response.cpp:162: v8::Local<v8::Value> wrapper = toV8(response, scriptState->context()->Global(), scriptState->isolate()); On 2016/01/30 15:35:53, haraken wrote: > > Shall we enter ScriptState::Scope before calling toV8? > > We are in the correct scope as this is called from the binding code with [CallWith=ScriptState]. https://codereview.chromium.org/1539803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/Response.cpp:162: v8::Local<v8::Value> wrapper = toV8(response, scriptState->context()->Global(), scriptState->isolate()); On 2016/01/30 15:35:53, haraken wrote: > > Can we use toV8(response, scriptState) ? Done.
https://codereview.chromium.org/1539803002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp (right): https://codereview.chromium.org/1539803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp:165: ReadableStreamOperations::read(m_scriptState.get(), reader).then( On 2016/02/02 10:59:28, yhirano wrote: > On 2016/01/30 15:35:53, haraken wrote: > > > > Would you create a helper function in V8ScriptRunner for this one? > > > > I want to encapsulate all the complexity about V8RecursionScope, Microtask etc > > into the helper functions in V8ScriptRunner. > > Do you have an idea how to encapsulate these calls without loss of generality? Maybe can we introduce V8CallPromiseScope to V8ScriptRunner.h so that we can write this as: V8CallPromiseScope scope(m_scriptState); ReadableStreamOperations::read(...).then(); All V8 calls that may run MicroTasks should be unified into V8ScriptRunner.h.
https://codereview.chromium.org/1539803002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp (right): https://codereview.chromium.org/1539803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp:165: ReadableStreamOperations::read(m_scriptState.get(), reader).then( On 2016/02/02 12:23:06, haraken wrote: > On 2016/02/02 10:59:28, yhirano wrote: > > On 2016/01/30 15:35:53, haraken wrote: > > > > > > Would you create a helper function in V8ScriptRunner for this one? > > > > > > I want to encapsulate all the complexity about V8RecursionScope, Microtask > etc > > > into the helper functions in V8ScriptRunner. > > > > Do you have an idea how to encapsulate these calls without loss of generality? > > Maybe can we introduce V8CallPromiseScope to V8ScriptRunner.h so that we can > write this as: > > V8CallPromiseScope scope(m_scriptState); > ReadableStreamOperations::read(...).then(); > > All V8 calls that may run MicroTasks should be unified into V8ScriptRunner.h. As we talked at https://codereview.chromium.org/1167343002/#msg45, I was wrong and we don't need V8RecursionScope here.
ping?
On 2016/02/05 14:52:04, yhirano wrote: > https://codereview.chromium.org/1539803002/diff/60001/third_party/WebKit/Sour... > File > third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp > (right): > > https://codereview.chromium.org/1539803002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp:165: > ReadableStreamOperations::read(m_scriptState.get(), reader).then( > On 2016/02/02 12:23:06, haraken wrote: > > On 2016/02/02 10:59:28, yhirano wrote: > > > On 2016/01/30 15:35:53, haraken wrote: > > > > > > > > Would you create a helper function in V8ScriptRunner for this one? > > > > > > > > I want to encapsulate all the complexity about V8RecursionScope, Microtask > > etc > > > > into the helper functions in V8ScriptRunner. > > > > > > Do you have an idea how to encapsulate these calls without loss of > generality? > > > > Maybe can we introduce V8CallPromiseScope to V8ScriptRunner.h so that we can > > write this as: > > > > V8CallPromiseScope scope(m_scriptState); > > ReadableStreamOperations::read(...).then(); > > > > All V8 calls that may run MicroTasks should be unified into V8ScriptRunner.h. > > As we talked at https://codereview.chromium.org/1167343002/#msg45, I was wrong > and we don't need V8RecursionScope here. I'm a bit confused... Is the ReadableStreamOperations::read an internal script? Or is it a user script? If it is an internal script, we need a MicroTaskSuppression. If it is a user function, we need a V8RecursionScope. Either way, I think we need either of MicroTaskSuppression or V8RecursionScope.
On 2016/02/12 04:31:51, haraken wrote: > On 2016/02/05 14:52:04, yhirano wrote: > > > https://codereview.chromium.org/1539803002/diff/60001/third_party/WebKit/Sour... > > File > > third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp > > (right): > > > > > https://codereview.chromium.org/1539803002/diff/60001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp:165: > > ReadableStreamOperations::read(m_scriptState.get(), reader).then( > > On 2016/02/02 12:23:06, haraken wrote: > > > On 2016/02/02 10:59:28, yhirano wrote: > > > > On 2016/01/30 15:35:53, haraken wrote: > > > > > > > > > > Would you create a helper function in V8ScriptRunner for this one? > > > > > > > > > > I want to encapsulate all the complexity about V8RecursionScope, > Microtask > > > etc > > > > > into the helper functions in V8ScriptRunner. > > > > > > > > Do you have an idea how to encapsulate these calls without loss of > > generality? > > > > > > Maybe can we introduce V8CallPromiseScope to V8ScriptRunner.h so that we can > > > write this as: > > > > > > V8CallPromiseScope scope(m_scriptState); > > > ReadableStreamOperations::read(...).then(); > > > > > > All V8 calls that may run MicroTasks should be unified into > V8ScriptRunner.h. > > > > As we talked at https://codereview.chromium.org/1167343002/#msg45, I was wrong > > and we don't need V8RecursionScope here. > > I'm a bit confused... > > Is the ReadableStreamOperations::read an internal script? Or is it a user > script? If it is an internal script, we need a MicroTaskSuppression. If it is a > user function, we need a V8RecursionScope. Either way, I think we need either of > MicroTaskSuppression or V8RecursionScope. As you said in https://codereview.chromium.org/1167343002/#msg46, ReadableStreamOperations::read is a mere call of V8ScriptRunner::callInternalFunction which contains MicrotaskSuppressionScope.
On 2016/02/12 06:43:47, yhirano wrote: > On 2016/02/12 04:31:51, haraken wrote: > > On 2016/02/05 14:52:04, yhirano wrote: > > > > > > https://codereview.chromium.org/1539803002/diff/60001/third_party/WebKit/Sour... > > > File > > > third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp > > > (right): > > > > > > > > > https://codereview.chromium.org/1539803002/diff/60001/third_party/WebKit/Sour... > > > > > > third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp:165: > > > ReadableStreamOperations::read(m_scriptState.get(), reader).then( > > > On 2016/02/02 12:23:06, haraken wrote: > > > > On 2016/02/02 10:59:28, yhirano wrote: > > > > > On 2016/01/30 15:35:53, haraken wrote: > > > > > > > > > > > > Would you create a helper function in V8ScriptRunner for this one? > > > > > > > > > > > > I want to encapsulate all the complexity about V8RecursionScope, > > Microtask > > > > etc > > > > > > into the helper functions in V8ScriptRunner. > > > > > > > > > > Do you have an idea how to encapsulate these calls without loss of > > > generality? > > > > > > > > Maybe can we introduce V8CallPromiseScope to V8ScriptRunner.h so that we > can > > > > write this as: > > > > > > > > V8CallPromiseScope scope(m_scriptState); > > > > ReadableStreamOperations::read(...).then(); > > > > > > > > All V8 calls that may run MicroTasks should be unified into > > V8ScriptRunner.h. > > > > > > As we talked at https://codereview.chromium.org/1167343002/#msg45, I was > wrong > > > and we don't need V8RecursionScope here. > > > > I'm a bit confused... > > > > Is the ReadableStreamOperations::read an internal script? Or is it a user > > script? If it is an internal script, we need a MicroTaskSuppression. If it is > a > > user function, we need a V8RecursionScope. Either way, I think we need either > of > > MicroTaskSuppression or V8RecursionScope. > > As you said in https://codereview.chromium.org/1167343002/#msg46, > ReadableStreamOperations::read is a mere call of > V8ScriptRunner::callInternalFunction which contains MicrotaskSuppressionScope. Ah, got it. LGTM.
lgtm https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp (right): https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp:136: static PassRefPtr<ReadingContext> create(ScriptState* scriptState, v8::Local<v8::Object> stream) streamReader
https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp (right): https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp:136: static PassRefPtr<ReadingContext> create(ScriptState* scriptState, v8::Local<v8::Object> stream) On 2016/02/12 at 07:24:21, tyoshino wrote: > streamReader Sorry. It's already fixed in the latest ps. Please ignore.
https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp (right): https://codereview.chromium.org/1539803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp:136: static PassRefPtr<ReadingContext> create(ScriptState* scriptState, v8::Local<v8::Object> stream) On 2016/02/12 07:25:05, tyoshino wrote: > On 2016/02/12 at 07:24:21, tyoshino wrote: > > streamReader > > Sorry. It's already fixed in the latest ps. Please ignore. Done.
The CQ bit was checked by yhirano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bashi@chromium.org, tyoshino@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1539803002/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1539803002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1539803002/120001
Message was sent while issue was closed.
Description was changed from ========== [Fetch API] Fix a memory leak with a Response constructed with a ReadableStream This CL fixes a memory leak with a Response constructed with a ReadableStream by holding a weak persistent in ReadableStreamDataConsumerHandle. The stream reader will be kept alive by the hidden reference in Response. BUG=564479 ========== to ========== [Fetch API] Fix a memory leak with a Response constructed with a ReadableStream This CL fixes a memory leak with a Response constructed with a ReadableStream by holding a weak persistent in ReadableStreamDataConsumerHandle. The stream reader will be kept alive by the hidden reference in Response. BUG=564479 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Fetch API] Fix a memory leak with a Response constructed with a ReadableStream This CL fixes a memory leak with a Response constructed with a ReadableStream by holding a weak persistent in ReadableStreamDataConsumerHandle. The stream reader will be kept alive by the hidden reference in Response. BUG=564479 ========== to ========== [Fetch API] Fix a memory leak with a Response constructed with a ReadableStream This CL fixes a memory leak with a Response constructed with a ReadableStream by holding a weak persistent in ReadableStreamDataConsumerHandle. The stream reader will be kept alive by the hidden reference in Response. BUG=564479 Committed: https://crrev.com/f1bb6e7acafff4116f8f6100f9612c93930c2ecc Cr-Commit-Position: refs/heads/master@{#375196} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/f1bb6e7acafff4116f8f6100f9612c93930c2ecc Cr-Commit-Position: refs/heads/master@{#375196} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
