Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
689713004:
Threaded data provider: Support main thread data notifications (Chrome side) (Closed)
|
Created:
6 years, 1 month ago by oystein (OOO til 10th of July) Modified:
5 years, 10 months ago Reviewers:
davidben CC:
chromium-reviews, darin-cc_chromium.org, jam, vsevik, Charlie Reis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThreaded data provider: Support main thread data notifications (Chrome side)
The threaded data receiver will now receive notifications about received data packets on the main thread (needed for the progress indicator to work properly), and can optionally specify that it wants to receive a full copy of the data (needed when the Inspector is attached).
If a threaded data receiver is attached to a resource request, we also now make sure the resource completed message is bounced via the background thread, to make sure all data is received on the main thread first.
Blink side patch: https://codereview.chromium.org/690793003
BUG=398076
Committed: https://crrev.com/0c7c18bfcea9d8c8b0f88558905221851d2b93ae
Cr-Commit-Position: refs/heads/master@{#314704}
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 9
Patch Set 3 : Review nits #
Total comments: 19
Patch Set 4 : Review fixes #
Total comments: 2
Patch Set 5 : Rebase #
Messages
Total messages: 21 (7 generated)
oysteine@chromium.org changed reviewers: + creis@chromium.org
Blink side is LGTM'ed now; time to put up the Chrome-side CL. creis: Would this be for you, or can you suggest someone else?
creis@chromium.org changed reviewers: + davidben@chromium.org - creis@chromium.org
Sorry, I was OOO. davidben@ is probably a better reviewer for ResourceDispatcher.
On 2015/01/21 23:49:01, Charlie Reis (slow until 1-29) wrote: > Sorry, I was OOO. davidben@ is probably a better reviewer for > ResourceDispatcher. I will look at this first thing tomorrow (EST) if I don't get to it by the end of the day. (Sorry about the delay.)
Would it be possible to add a unit test for this ThreadedDataReceiver machinery? There's a lot of little behaviors we should probably test (e.g. what if the request is canceled while OnRequestCompleted is being flushed?). Higher level than this CL, but I'm also a little puzzled by the context. From skimming the original CLs, it looks like ThreadedDataReceiver is to re-route data from the WebURLLoaderClient to a different object. Before that, how did the data get from the ResourceDispatcher to the parser thread? I'm guessing there was a Blink-internal thread hop. Interface-wise, wouldn't it be cleaner to have kept the data flowing through WebURLLoaderClient and do the thread-hop somewhere in the WebURLLoaderClient implementation, or is there some reason that making ResourceDispatcher aware of this is better? https://codereview.chromium.org/689713004/diff/20001/content/child/resource_d... File content/child/resource_dispatcher.cc (right): https://codereview.chromium.org/689713004/diff/20001/content/child/resource_d... content/child/resource_dispatcher.cc:603: void ResourceDispatcher::CompletedRequestAfterBackgroundThreadFlush( Huh. I never failed a separate bug for this, but would this fix the parser half of this guy too? https://code.google.com/p/chromium/issues/detail?id=422246#c5 Though it seems weird to me, design-wise, that we're relying on ThreadedDataProvider being "done" with its processing in one background thread event loop iteration. (As opposed to the ThreadedDataProvider getting the completion signal, flushing whatever it needs to flush, and then informing the WebURLLoaderClient outside ResourceDispatcher.) https://codereview.chromium.org/689713004/diff/20001/content/child/threaded_d... File content/child/threaded_data_provider.cc (right): https://codereview.chromium.org/689713004/diff/20001/content/child/threaded_d... content/child/threaded_data_provider.cc:315: if (threaded_data_receiver_->needsMainthreadDataCopy()) { Can needsMainthreadDataCopy be called on both threads? (Here and line 334.) https://codereview.chromium.org/689713004/diff/20001/content/child/threaded_d... content/child/threaded_data_provider.cc:334: DCHECK(!data_copy || threaded_data_receiver_->needsMainthreadDataCopy()); Nit: I'd maybe also DCHECK something like if (data_copy) DCHECK_EQ(data_length, data->size()); (Probably will need to cast something to size_t.) https://codereview.chromium.org/689713004/diff/20001/content/child/threaded_d... content/child/threaded_data_provider.cc:337: data_copy ? data_copy->data() : NULL, data_length, encoded_data_length); std::vector::data is a C++11-ism. I don't think all of our STLs have it yet. (Notably stlport on Android.)
On 2015/01/23 19:46:43, David Benjamin wrote: > Would it be possible to add a unit test for this ThreadedDataReceiver machinery? > There's a lot of little behaviors we should probably test (e.g. what if the > request is canceled while OnRequestCompleted is being flushed?). > There's already end-to-end tests which covers pretty much anything the ThreadedDataProvider stuff is involved with; including layout tests I've added when we I've found specific bugs that's triggered by the direct I/O thread -> Parserthread shortcut (see the Blink-side of the CL). I'm not sure if a unittest specifically of this class is worth the complexity, given that it's all about shuffling data between three threads. I can give it a try if you think it would be helpful though. > Higher level than this CL, but I'm also a little puzzled by the context. From > skimming the original CLs, it looks like ThreadedDataReceiver is to re-route > data from the WebURLLoaderClient to a different object. Before that, how did the > data get from the ResourceDispatcher to the parser thread? I'm guessing there > was a Blink-internal thread hop. Interface-wise, wouldn't it be cleaner to have > kept the data flowing through WebURLLoaderClient and do the thread-hop somewhere > in the WebURLLoaderClient implementation, or is there some reason that making > ResourceDispatcher aware of this is better? The main purpose of ThreadedDataReceiver is actually to let the resource data flow directly from the I/O thread to the background (parser) thread. The only reason there's a path where data flows from the WebURLLoaderClient to the ThreadedDataReceiver, is for any data which is in-flight between the I/O thread and the main thread, at the point when the IPC message filter gets installed on the I/O thread. After that's flushed, WebURLLoaderClient isn't touched at all (that's main-thread only, which we completely bypass at that point). https://codereview.chromium.org/689713004/diff/20001/content/child/resource_d... File content/child/resource_dispatcher.cc (right): https://codereview.chromium.org/689713004/diff/20001/content/child/resource_d... content/child/resource_dispatcher.cc:603: void ResourceDispatcher::CompletedRequestAfterBackgroundThreadFlush( On 2015/01/23 19:46:43, David Benjamin wrote: > Huh. I never failed a separate bug for this, but would this fix the parser half > of this guy too? > https://code.google.com/p/chromium/issues/detail?id=422246#c5 > Interesting! It actually might, though it depends on what is actually stopping the parser. If anything triggered by ResourceMsg_RequestComplete, it would. > Though it seems weird to me, design-wise, that we're relying on > ThreadedDataProvider being "done" with its processing in one background thread > event loop iteration. (As opposed to the ThreadedDataProvider getting the > completion signal, flushing whatever it needs to flush, and then informing the > WebURLLoaderClient outside ResourceDispatcher.) Well, we can safely rely on it since we can assume that the RequestComplete IPC will be received on the I/O thread after any of the actual data IPCs, hence this call (which will bounce from the I/O -> Mainthread -> Parserthread) will be processed after the data which bounces I/O -> Parserthread directly. https://codereview.chromium.org/689713004/diff/20001/content/child/threaded_d... File content/child/threaded_data_provider.cc (right): https://codereview.chromium.org/689713004/diff/20001/content/child/threaded_d... content/child/threaded_data_provider.cc:315: if (threaded_data_receiver_->needsMainthreadDataCopy()) { On 2015/01/23 19:46:43, David Benjamin wrote: > Can needsMainthreadDataCopy be called on both threads? (Here and line 334.) Yep, it's threadsafe. https://codereview.chromium.org/689713004/diff/20001/content/child/threaded_d... content/child/threaded_data_provider.cc:334: DCHECK(!data_copy || threaded_data_receiver_->needsMainthreadDataCopy()); On 2015/01/23 19:46:43, David Benjamin wrote: > Nit: I'd maybe also DCHECK something like > if (data_copy) > DCHECK_EQ(data_length, data->size()); > (Probably will need to cast something to size_t.) Done. https://codereview.chromium.org/689713004/diff/20001/content/child/threaded_d... content/child/threaded_data_provider.cc:337: data_copy ? data_copy->data() : NULL, data_length, encoded_data_length); On 2015/01/23 19:46:43, David Benjamin wrote: > std::vector::data is a C++11-ism. I don't think all of our STLs have it yet. > (Notably stlport on Android.) Done.
> There's already end-to-end tests which covers pretty much anything the > ThreadedDataProvider stuff is involved with; including layout tests I've added > when we I've found specific bugs that's triggered by the direct I/O thread -> > Parserthread shortcut (see the Blink-side of the CL). I'm not sure if a unittest > specifically of this class is worth the complexity, given that it's all about > shuffling data between three threads. I can give it a try if you think it would > be helpful though. I'm thinking maybe testing the ResourceDispatcher+ThreadedDataProvider assembly, but with the test supplying the input IPC messages and WebThreadedDataReceiver / WebURLLoaderClient. There's a lot of cases that probably would be worth testing explicitly, which an end-to-end test probably doesn't have as much control over. Making sure both data paths get triggered, testing what happens if the request aborts or is canceled before the filter is attached, the various shutdown cases, etc. Especially since, on a closer look, there's a lot of subtle ordering in this code. If this ends up being a mess, I don't want to insist on a ton of busywork, but this does seem to be a relatively subtle bit of code that's fairly well-isolated. > The main purpose of ThreadedDataReceiver is actually to let the resource data > flow directly from the I/O thread to the background (parser) thread. The only > reason there's a path where data flows from the WebURLLoaderClient to the > ThreadedDataReceiver, is for any data which is in-flight between the I/O thread > and the main thread, at the point when the IPC message filter gets installed on > the I/O thread. After that's flushed, WebURLLoaderClient isn't touched at all > (that's main-thread only, which we completely bypass at that point). Oh, I see. Somehow I'd missed that there was a message filter there. I understand the motivation now. Thanks for explaining! (That might be worth an explanatory comment in ThreadedDataReceiver's header.) https://codereview.chromium.org/689713004/diff/20001/content/child/resource_d... File content/child/resource_dispatcher.cc (right): https://codereview.chromium.org/689713004/diff/20001/content/child/resource_d... content/child/resource_dispatcher.cc:603: void ResourceDispatcher::CompletedRequestAfterBackgroundThreadFlush( On 2015/01/23 21:23:47, Oystein wrote: > On 2015/01/23 19:46:43, David Benjamin wrote: > > Huh. I never failed a separate bug for this, but would this fix the parser > half > > of this guy too? > > https://code.google.com/p/chromium/issues/detail?id=422246#c5 > > > Interesting! It actually might, though it depends on what is actually stopping > the parser. If anything triggered by ResourceMsg_RequestComplete, it would. Yeah, I think that stop was triggered by ResourceComplete. https://codereview.chromium.org/689713004/diff/40001/content/child/threaded_d... File content/child/threaded_data_provider.cc (right): https://codereview.chromium.org/689713004/diff/40001/content/child/threaded_d... content/child/threaded_data_provider.cc:192: } If current_background_thread is NULL, this looks like it leaks. https://codereview.chromium.org/689713004/diff/40001/content/child/threaded_d... content/child/threaded_data_provider.cc:204: background_thread_weak_factory_.reset(NULL); Instead of a scoped_ptr<WeakFactory>, why not call InvalidateWeakPtrs() at this point? https://codereview.chromium.org/689713004/diff/40001/content/child/threaded_d... content/child/threaded_data_provider.cc:246: background_thread_weak_factory_->GetWeakPtr())); Why does this one call GetWeakPtr() while the others use Unretained? https://codereview.chromium.org/689713004/diff/40001/content/child/threaded_d... content/child/threaded_data_provider.cc:314: scoped_ptr<std::vector<char> > data_copy; Nit: > > can be >> now. https://codereview.chromium.org/689713004/diff/40001/content/child/threaded_d... content/child/threaded_data_provider.cc:317: memcpy(data_copy->data(), data, data_length); Another std::vector::data. How about using the iterator constructor of std::vector and do data_copy.reset(new std::vector<char>(data, data + data_length)); https://codereview.chromium.org/689713004/diff/40001/content/child/threaded_d... content/child/threaded_data_provider.cc:322: base::Unretained(this), It's not immediately obvious why this is safe (ditto with the other base::Unretaineds). It looks like it's because ThreadedDataProvider isn't actually owned by anything. Instead its "owner" calls Stop() on it, which then cycles to/from the background thread to destroy it. And one of the contracts is that no new main-thread methods are called after Stop(), which then ensures that nothing can be called on the background thread after StopOnBackgroundThread(). This is subtle enough to probably warrant a comment in the header. ~ThreadedDataProvider should probably also be private, to ensure that nothing else deletes it. https://codereview.chromium.org/689713004/diff/40001/content/child/threaded_d... content/child/threaded_data_provider.cc:340: data_copy ? &data_copy->front() : NULL, data_length, encoded_data_length); Sadly, that's also a problem if data_copy->size() is zero. :-( Not having std::vector::data can be kind of a pain. I think you want (data_copy && data_copy->size()) ? &data_copy[0] : nullptr https://codereview.chromium.org/689713004/diff/40001/content/child/threaded_d... File content/child/threaded_data_provider.h (right): https://codereview.chromium.org/689713004/diff/40001/content/child/threaded_d... content/child/threaded_data_provider.h:39: virtual ~ThreadedDataProvider(); Why is this virtual? Does anything subclass it?
On 2015/01/26 21:53:13, David Benjamin wrote: > > There's already end-to-end tests which covers pretty much anything the > > ThreadedDataProvider stuff is involved with; including layout tests I've added > > when we I've found specific bugs that's triggered by the direct I/O thread -> > > Parserthread shortcut (see the Blink-side of the CL). I'm not sure if a > unittest > > specifically of this class is worth the complexity, given that it's all about > > shuffling data between three threads. I can give it a try if you think it > would > > be helpful though. > > I'm thinking maybe testing the ResourceDispatcher+ThreadedDataProvider assembly, > but with the test supplying the input IPC messages and WebThreadedDataReceiver / > WebURLLoaderClient. There's a lot of cases that probably would be worth testing > explicitly, which an end-to-end test probably doesn't have as much control over. > Making sure both data paths get triggered, testing what happens if the request > aborts or is canceled before the filter is attached, the various shutdown cases, > etc. Especially since, on a closer look, there's a lot of subtle ordering in > this code. > > If this ends up being a mess, I don't want to insist on a ton of busywork, but > this does seem to be a relatively subtle bit of code that's fairly > well-isolated. The more I look back at this code the more I'm reminded of why I didn't write any dedicated unittest for this class in the original CLs and rather relied on end-to-end layout tests etc; juggling the three threads, IPCs, setting up shared memory buffers for the resources... the complexity of a self-contained test would outweigh the class itself and I'm pretty sure in the end the test would mainly be testing itself. I'm deeply hesitant to spending a week (or more) on this at this point. https://codereview.chromium.org/689713004/diff/40001/content/child/threaded_d... File content/child/threaded_data_provider.cc (right): https://codereview.chromium.org/689713004/diff/40001/content/child/threaded_d... content/child/threaded_data_provider.cc:192: } On 2015/01/26 21:53:12, David Benjamin wrote: > If current_background_thread is NULL, this looks like it leaks. Yes. This only happens on shutdown, due to the ResourceScheduler being shut down after Blink is. https://codereview.chromium.org/689713004/diff/40001/content/child/threaded_d... content/child/threaded_data_provider.cc:204: background_thread_weak_factory_.reset(NULL); On 2015/01/26 21:53:12, David Benjamin wrote: > Instead of a scoped_ptr<WeakFactory>, why not call InvalidateWeakPtrs() at this > point? The WeakPtrFactory has to be destructed on the thread it's sequenced to (i.e. the one that validates/uses its issued WeakPtrs). https://codereview.chromium.org/689713004/diff/40001/content/child/threaded_d... content/child/threaded_data_provider.cc:246: background_thread_weak_factory_->GetWeakPtr())); On 2015/01/26 21:53:12, David Benjamin wrote: > Why does this one call GetWeakPtr() while the others use Unretained? With this one there's a chance that the ThreadedDataProvider could be shut down while the task is in flight, not so for the ones using Unretained (they're both Mainthread -> Backgroundthread, i.e. the same path the destruction calls happen). https://codereview.chromium.org/689713004/diff/40001/content/child/threaded_d... content/child/threaded_data_provider.cc:314: scoped_ptr<std::vector<char> > data_copy; On 2015/01/26 21:53:12, David Benjamin wrote: > Nit: > > can be >> now. Done. https://codereview.chromium.org/689713004/diff/40001/content/child/threaded_d... content/child/threaded_data_provider.cc:317: memcpy(data_copy->data(), data, data_length); On 2015/01/26 21:53:12, David Benjamin wrote: > Another std::vector::data. How about using the iterator constructor of > std::vector and do > > data_copy.reset(new std::vector<char>(data, data + data_length)); Done. https://codereview.chromium.org/689713004/diff/40001/content/child/threaded_d... content/child/threaded_data_provider.cc:322: base::Unretained(this), On 2015/01/26 21:53:12, David Benjamin wrote: > It's not immediately obvious why this is safe (ditto with the other > base::Unretaineds). It looks like it's because ThreadedDataProvider isn't > actually owned by anything. Instead its "owner" calls Stop() on it, which then > cycles to/from the background thread to destroy it. And one of the contracts is > that no new main-thread methods are called after Stop(), which then ensures that > nothing can be called on the background thread after StopOnBackgroundThread(). > > This is subtle enough to probably warrant a comment in the header. > ~ThreadedDataProvider should probably also be private, to ensure that nothing > else deletes it. Done. https://codereview.chromium.org/689713004/diff/40001/content/child/threaded_d... content/child/threaded_data_provider.cc:340: data_copy ? &data_copy->front() : NULL, data_length, encoded_data_length); On 2015/01/26 21:53:12, David Benjamin wrote: > Sadly, that's also a problem if data_copy->size() is zero. :-( Not having > std::vector::data can be kind of a pain. I think you want > > (data_copy && data_copy->size()) ? &data_copy[0] : nullptr Done. https://codereview.chromium.org/689713004/diff/40001/content/child/threaded_d... File content/child/threaded_data_provider.h (right): https://codereview.chromium.org/689713004/diff/40001/content/child/threaded_d... content/child/threaded_data_provider.h:39: virtual ~ThreadedDataProvider(); On 2015/01/26 21:53:13, David Benjamin wrote: > Why is this virtual? Does anything subclass it? Not anymore; leftover from a prior draft I think.
Patchset #4 (id:60001) has been deleted
I'd still be happier with a unit test, but I won't block on it, so lgtm as long as this new case is covered by the end-to-end tests in Blink. Maybe I'll try my hand at testing this later when I have a spare cycle. :-) To avoid adding to the scope creep---my fault! sorry about that---the comments below are for follow-ups. https://codereview.chromium.org/689713004/diff/40001/content/child/threaded_d... File content/child/threaded_data_provider.cc (right): https://codereview.chromium.org/689713004/diff/40001/content/child/threaded_d... content/child/threaded_data_provider.cc:192: } On 2015/01/26 23:49:04, Oystein wrote: > On 2015/01/26 21:53:12, David Benjamin wrote: > > If current_background_thread is NULL, this looks like it leaks. > > Yes. This only happens on shutdown, due to the ResourceScheduler being shut down > after Blink is. Hrm. That feels like there might be a deeper ownership problem here... (wouldn't Blink tear down all its WebURLLoaders before then?), but let's leave that to a follow-up, if any. https://codereview.chromium.org/689713004/diff/40001/content/child/threaded_d... content/child/threaded_data_provider.cc:204: background_thread_weak_factory_.reset(NULL); On 2015/01/26 23:49:04, Oystein wrote: > On 2015/01/26 21:53:12, David Benjamin wrote: > > Instead of a scoped_ptr<WeakFactory>, why not call InvalidateWeakPtrs() at > this > > point? > > The WeakPtrFactory has to be destructed on the thread it's sequenced to (i.e. > the one that validates/uses its issued WeakPtrs). Hrm. You sure? I think it's just that each batch of WeakPtrs needs to be invalidated on the thread it's sequenced to. Invalidate drops the WeakReference::Flag which is the thing that actually has the threading guarantees. Even if the factory were to issue a new batch of WeakPtrs on a different thread, I believe that would be fine as long as the old generation and previous generation were correctly ordered. There's even a test for it: https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p... Anyway, scope creep. Let's leave this for a follow-up, if any. https://codereview.chromium.org/689713004/diff/40001/content/child/threaded_d... content/child/threaded_data_provider.cc:246: background_thread_weak_factory_->GetWeakPtr())); On 2015/01/26 23:49:04, Oystein wrote: > On 2015/01/26 21:53:12, David Benjamin wrote: > > Why does this one call GetWeakPtr() while the others use Unretained? > > With this one there's a chance that the ThreadedDataProvider could be shut down > while the task is in flight, not so for the ones using Unretained (they're both > Mainthread -> Backgroundthread, i.e. the same path the destruction calls > happen). (Also for follow-up, if anything) Isn't this one also Mainthread -> Backgroundthread? https://codereview.chromium.org/689713004/diff/80001/content/child/threaded_d... File content/child/threaded_data_provider.cc (left): https://codereview.chromium.org/689713004/diff/80001/content/child/threaded_d... content/child/threaded_data_provider.cc:160: void DestructOnMainThread(ThreadedDataProvider* data_provider) { (Huh. Just noticed the old one here wasn't in the anonymous namespace. :-) ) https://codereview.chromium.org/689713004/diff/80001/content/child/threaded_d... File content/child/threaded_data_provider.h (right): https://codereview.chromium.org/689713004/diff/80001/content/child/threaded_d... content/child/threaded_data_provider.h:43: void OnReceivedDataOnBackgroundThread(int data_offset, Nit: (For a follow-up, since the order was off before the CL.) Could you fix up the order of the .cc file to match the header in a follow-up change?
On 2015/01/27 21:33:25, David Benjamin wrote: > I'd still be happier with a unit test, but I won't block on it, so lgtm as long > as this new case is covered by the end-to-end tests in Blink. Maybe I'll try my > hand at testing this later when I have a spare cycle. :-) > Thanks! Yeah the Blink-side of this CL has an Inspector layout test which covers this thing. And an attempt at a test would be cool, maybe it's less onerous than I think :). > To avoid adding to the scope creep---my fault! sorry about that---the comments > below are for follow-ups. > Gotcha thanks; I'll go through these next time I touch this. > https://codereview.chromium.org/689713004/diff/40001/content/child/threaded_d... > File content/child/threaded_data_provider.cc (right): > > https://codereview.chromium.org/689713004/diff/40001/content/child/threaded_d... > content/child/threaded_data_provider.cc:192: } > On 2015/01/26 23:49:04, Oystein wrote: > > On 2015/01/26 21:53:12, David Benjamin wrote: > > > If current_background_thread is NULL, this looks like it leaks. > > > > Yes. This only happens on shutdown, due to the ResourceScheduler being shut > down > > after Blink is. > > Hrm. That feels like there might be a deeper ownership problem here... (wouldn't > Blink tear down all its WebURLLoaders before then?), but let's leave that to a > follow-up, if any. > > https://codereview.chromium.org/689713004/diff/40001/content/child/threaded_d... > content/child/threaded_data_provider.cc:204: > background_thread_weak_factory_.reset(NULL); > On 2015/01/26 23:49:04, Oystein wrote: > > On 2015/01/26 21:53:12, David Benjamin wrote: > > > Instead of a scoped_ptr<WeakFactory>, why not call InvalidateWeakPtrs() at > > this > > > point? > > > > The WeakPtrFactory has to be destructed on the thread it's sequenced to (i.e. > > the one that validates/uses its issued WeakPtrs). > > Hrm. You sure? I think it's just that each batch of WeakPtrs needs to be > invalidated on the thread it's sequenced to. Invalidate drops the > WeakReference::Flag which is the thing that actually has the threading > guarantees. Even if the factory were to issue a new batch of WeakPtrs on a > different thread, I believe that would be fine as long as the old generation and > previous generation were correctly ordered. There's even a test for it: > > https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p... > > Anyway, scope creep. Let's leave this for a follow-up, if any. > I won't swear to it, but I'm pretty sure this was asserting before. I went back and checked; the original first revisions of this feature had the WeakPtrFactory as a normal member. Maybe the WeakPtr behavior has changed in the meantime? > https://codereview.chromium.org/689713004/diff/40001/content/child/threaded_d... > content/child/threaded_data_provider.cc:246: > background_thread_weak_factory_->GetWeakPtr())); > On 2015/01/26 23:49:04, Oystein wrote: > > On 2015/01/26 21:53:12, David Benjamin wrote: > > > Why does this one call GetWeakPtr() while the others use Unretained? > > > > With this one there's a chance that the ThreadedDataProvider could be shut > down > > while the task is in flight, not so for the ones using Unretained (they're > both > > Mainthread -> Backgroundthread, i.e. the same path the destruction calls > > happen). > > (Also for follow-up, if anything) Isn't this one also Mainthread -> > Backgroundthread? > > https://codereview.chromium.org/689713004/diff/80001/content/child/threaded_d... > File content/child/threaded_data_provider.cc (left): > > https://codereview.chromium.org/689713004/diff/80001/content/child/threaded_d... > content/child/threaded_data_provider.cc:160: void > DestructOnMainThread(ThreadedDataProvider* data_provider) { > (Huh. Just noticed the old one here wasn't in the anonymous namespace. :-) ) > > https://codereview.chromium.org/689713004/diff/80001/content/child/threaded_d... > File content/child/threaded_data_provider.h (right): > > https://codereview.chromium.org/689713004/diff/80001/content/child/threaded_d... > content/child/threaded_data_provider.h:43: void > OnReceivedDataOnBackgroundThread(int data_offset, > Nit: (For a follow-up, since the order was off before the CL.) Could you fix up > the order of the .cc file to match the header in a follow-up change?
The CQ bit was checked by oysteine@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/689713004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...) ios_rel_device_ninja_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
Patchset #5 (id:100001) has been deleted
The CQ bit was checked by oysteine@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/689713004/120001
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0c7c18bfcea9d8c8b0f88558905221851d2b93ae Cr-Commit-Position: refs/heads/master@{#314704} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
