|
|
Created:
9 years, 2 months ago by Joao da Silva Modified:
9 years, 2 months ago Reviewers:
commit-bot: I haz the power, Paweł Hajdan Jr., jam, willchan no longer on Chromium, Nirnimesh CC:
chromium-reviews, kkania Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionWait for URLBlacklist update tasks on automation tests.
Also introduced BrowserThread::WaitForPendingTasksOn(), and removing SignalingTask.
BUG=100217
TEST=policy.PolicyTest.testBlacklistPolicy doesn't flake
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107113
Patch Set 1 #
Total comments: 8
Patch Set 2 : Added BrowserThread::WaitForPendingTasksOn, removed SignalingTask #
Total comments: 1
Patch Set 3 : Removed BrowserThread:: extensions, simpler test, re-enabled testBlacklistPolicy pyauto test #Patch Set 4 : Rebased #
Total comments: 1
Patch Set 5 : Reviewed #
Total comments: 3
Patch Set 6 : Non-blocking version of loop flush #Patch Set 7 : Added CHECKs #
Total comments: 11
Patch Set 8 : Reviewed: removed CHECKs, added comment, nits #
Total comments: 2
Patch Set 9 : Rebased #Patch Set 10 : rebase #
Messages
Total messages: 30 (0 generated)
Hey Nirnimesh, there is a very remote possibility that the blacklist hasn't been updated yet before NavigateToURL is called, and this fixes it. The update of that policy requires posting a task to IO to grab a WeakPtr, posting another task to FILE to do some work, and posting back to IO to check the WeakPtr and update some state based on the output of the FILE task. So waiting for those message loops to flush is enough to prevent that race; I'm not sure this CL is the best approach, so please let me know if there is a better way to do it.
http://codereview.chromium.org/8274012/diff/1/chrome/browser/automation/testi... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/8274012/diff/1/chrome/browser/automation/testi... chrome/browser/automation/testing_automation_provider.cc:181: class SignalingTask : public Task { Huh? This is duplicating an existing class with the same name. Please share it. http://codereview.chromium.org/8274012/diff/1/chrome/browser/automation/testi... chrome/browser/automation/testing_automation_provider.cc:195: // Waits until all pending tasks on |id| are executed. We have lots of existing code that does the same thing. Make sure to share the code. http://codereview.chromium.org/8274012/diff/1/chrome/browser/automation/testi... chrome/browser/automation/testing_automation_provider.cc:197: CHECK(!BrowserThread::CurrentlyOn(id)); Instead of CHECKS, I really think you should return a bool. http://codereview.chromium.org/8274012/diff/1/chrome/browser/automation/testi... chrome/browser/automation/testing_automation_provider.cc:201: event.Wait(); This has a return value, please check it.
Adding @sky for an owner review of content/browser. If this is acceptable to the current reviewers I'll then run it through the owners of the other places. @nirnimesh: please review the fix for the automation test @phajdan.jr: please have another look @sky: introduced BrowserThread::WaitForPendingTasksOn(id). wdyt? http://codereview.chromium.org/8274012/diff/1/chrome/browser/automation/testi... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/8274012/diff/1/chrome/browser/automation/testi... chrome/browser/automation/testing_automation_provider.cc:181: class SignalingTask : public Task { On 2011/10/13 20:10:59, Paweł Hajdan Jr. wrote: > Huh? This is duplicating an existing class with the same name. Please share it. Done. http://codereview.chromium.org/8274012/diff/1/chrome/browser/automation/testi... chrome/browser/automation/testing_automation_provider.cc:195: // Waits until all pending tasks on |id| are executed. On 2011/10/13 20:10:59, Paweł Hajdan Jr. wrote: > We have lots of existing code that does the same thing. Make sure to share the > code. Done. http://codereview.chromium.org/8274012/diff/1/chrome/browser/automation/testi... chrome/browser/automation/testing_automation_provider.cc:197: CHECK(!BrowserThread::CurrentlyOn(id)); On 2011/10/13 20:10:59, Paweł Hajdan Jr. wrote: > Instead of CHECKS, I really think you should return a bool. Done. http://codereview.chromium.org/8274012/diff/1/chrome/browser/automation/testi... chrome/browser/automation/testing_automation_provider.cc:201: event.Wait(); On 2011/10/13 20:10:59, Paweł Hajdan Jr. wrote: > This has a return value, please check it. Wait() is void. A boolean is returned by TimedWait though.
I think this is scary, and at a minimum you should DCHECK that not on the UI thread. Jon is a better reviewer for this than I though. -Scott
http://codereview.chromium.org/8274012/diff/1003/content/browser/browser_thre... File content/browser/browser_thread.h (right): http://codereview.chromium.org/8274012/diff/1003/content/browser/browser_thre... content/browser/browser_thread.h:176: static bool WaitForPendingTasksOn(ID identifier); this is most certainly a function that we don't want on BrowserThread. any such function, once it's in, would be misused. check out http://dev.chromium.org/developers/design-documents/threading "Chromium is a very multithreaded product. We try to keep the UI as responsive as possible, and this means not blocking the UI thread with any blocking I/O or other expensive operations. Our approach is to use message passing as the way of communicating between threads. We discourage locking and threadsafe objects. Instead, objects live on only one thread, we pass messages between threads for communication, and we use callback interfaces (implemented by message passing) for most cross-thread requests." I don't know the details of the test that you're trying to fix, but it seems that you can do this waiting in the test code yourself, without touching the content code.
Removed the changes in content/, and @sky and @jam as reviewers. @nirnimesh: please have another look @phajdan: please have another look too. This is similar to the initial CL. chrome/test/base/signaling_task.h can't be used because it isn't linked with the browser target, and it's not moved there because of the potential for misuse, as suggested by @jam. Somehow the try bots running pyauto_functional_tests don't show up here. Here are the builders for the last patch set: http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/54977 http://build.chromium.org/p/tryserver.chromium/builders/win/builds/61701
http://codereview.chromium.org/8274012/diff/11001/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/8274012/diff/11001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:181: CHECK(!BrowserThread::CurrentlyOn(identifier)); I commented about those lines. Please apply that comment. It was about not using CHECKs in test code but returning bool instead.
> It was about not using CHECKs in test code but returning bool instead. Maybe it's a dumb question, but can you tell me why you insist on this? Having CHECKs on provider side seems fine to me -- the rest of the chrome code certainly does so disallowing in provider doesn't buy you anything.
LGTM
just a sec: I thought this would be done in tests only. But this code is part of chrome. I don't think we want to have non-test code make one thread wait on another. Can this be used by chrome frame for example? or is this method strictly only used for testing? it's dangerous to add code like this because other non-test code can copy this pattern. why do this instead of asynchronously waiting or replying?
On 2011/10/18 18:28:58, John Abd-El-Malek wrote: > just a sec: I thought this would be done in tests only. But this code is part of > chrome. I don't think we want to have non-test code make one thread wait on > another. Can this be used by chrome frame for example? or is this method > strictly only used for testing? testing_automation_provider.cc is strictly test only. > > it's dangerous to add code like this because other non-test code can copy this > pattern. why do this instead of asynchronously waiting or replying?
(sorry for the delay, I missed this) On 2011/10/18 18:36:22, Nirnimesh wrote: > On 2011/10/18 18:28:58, John Abd-El-Malek wrote: > > just a sec: I thought this would be done in tests only. But this code is part > of > > chrome. I don't think we want to have non-test code make one thread wait on > > another. Can this be used by chrome frame for example? or is this method > > strictly only used for testing? > > testing_automation_provider.cc is strictly test only. I see. I'm still worried about putting code like this in a non-test file, in case people copy it to other code which runs in the browser process. Can we do this just in the test file? > > > > > it's dangerous to add code like this because other non-test code can copy this > > pattern. why do this instead of asynchronously waiting or replying?
Nirnimesh: CHECK or DCHECK crashes the entire browser, and it's especially painful under ui_tests. The error messages are frequently poor because of Windows logging quirks and missing symbols on the bots. I strongly prefer returning error code/bool to the automation client (so that at least you can have _some_ idea what failed) instead of crashing the browser (which is painful to debug). John: I don't see a way to _move_ the code in a way you requested. TestingAutomationProvider is sort of indeed testing-only, but it has to be a part of the browser (one possible way out is implementing a separate DLL as Darin suggested). Maybe there is other way of implementing the code though that doesn't block.I'm frequently worried about suspicious code patterns too. http://codereview.chromium.org/8274012/diff/11003/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/8274012/diff/11003/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:194: event.Wait(); I think this also returns bool, so please return event.Wait in that case. Otherwise, please ignore.
On 2011/10/19 09:20:02, Paweł Hajdan Jr. wrote: > Nirnimesh: CHECK or DCHECK crashes the entire browser, and it's especially > painful under ui_tests. The error messages are frequently poor because of > Windows logging quirks and missing symbols on the bots. I strongly prefer > returning error code/bool to the automation client (so that at least you can > have _some_ idea what failed) instead of crashing the browser (which is painful > to debug). I understand that crashing can get painful in the context of ui_tests on the client side. My question was about CHECKs on the provider side. Provider makes calls into chrome, which in turn uses CHECKs anyway, so I don't see much value in forbidding it in testing_automation_provider.cc. Or did I misunderstand?
On 2011/10/19 10:30:38, Nirnimesh wrote: > I understand that crashing can get painful in the context of ui_tests on the > client side. > My question was about CHECKs on the provider side. Provider makes calls into > chrome, which in turn uses CHECKs anyway, so I don't see much value in > forbidding it in testing_automation_provider.cc. Or did I misunderstand? Good question. My idea is that CHECKs in the browser code are "legitimate" and unavoidable anyway. However, the testing infrastructure should really rather return an error in error condition and not crash the browser. For example, why PostTask doesn't CHECK itself but returns a bool parameter? I think our automation code should do the safe and forward the error result to the automation client instead of crashing.
> Good question. My idea is that CHECKs in the browser code are "legitimate" and > unavoidable anyway. However, the testing infrastructure should really rather > return an error in error condition and not crash the browser. That's a judgement call. > For example, why > PostTask doesn't CHECK itself but returns a bool parameter? In cases where chrome code returns bool, like this one, yes. > I think our > automation code should do the safe and forward the error result to the > automation client instead of crashing. But in cases where it's too cumbersome to account for each and every condition, it's better to CHECK. Why? For the same reason why chrome prefers to crash (by CHECK) than work around the problem with extensive error checks. In any case, I think this CL is now ready for commit.
On 2011/10/19 09:20:02, Paweł Hajdan Jr. wrote: > John: I don't see a way to _move_ the code in a way you requested. > TestingAutomationProvider is sort of indeed testing-only, but it has to be a > part of the browser (one possible way out is implementing a separate DLL as > Darin suggested). Maybe there is other way of implementing the code though that > doesn't block.I'm frequently worried about suspicious code patterns too. On 2011/10/19 19:11:00, Nirnimesh wrote: > In any case, I think this CL is now ready for commit. I don't think this is ready to commit at this point. I understand this code is only used for testing, but the chrome style guide (both in the nitpicking spaces/tabs and also higher level patterns) applies to test code as well. In both cases where this change blocks one thread on another, we can convert this to tasks that operate asynchronously. http://codereview.chromium.org/8274012/diff/11003/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/8274012/diff/11003/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:2902: CHECK(WaitForPendingTasksOn(BrowserThread::IO)); instead of blocking one thread on another, why not post a task to the other thread and then have that post a task back to this thread, and then that second task is what sends the reply message? this is the pattern that we use all over chrome http://codereview.chromium.org/8274012/diff/11003/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:5921: CHECK(WaitForPendingTasksOn(BrowserThread::IO)); ditto
On 2011/10/19 19:11:00, Nirnimesh wrote: > > Good question. My idea is that CHECKs in the browser code are "legitimate" and > > unavoidable anyway. However, the testing infrastructure should really rather > > return an error in error condition and not crash the browser. > > That's a judgement call. To the same degree as any other design question is. I think it's reasonable (and arguable, but not sure if it's worth it) to say that automation should return an error code rather than crash the browser. Actually I'm really temped to say "after all, if you want to make your tests more crashy and harder to debug, go ahead and do it", but I don't want other people to copy the pattern and say "see, here is an example that does the same". > But in cases where it's too cumbersome to account for each and every condition, > it's better to CHECK. Why? For the same reason why chrome prefers to crash (by > CHECK) than work around the problem with extensive error checks. Sorry, I don't get it. Are you saying that for bools it's OK to forward the error code (and iirc the code in question returns bools), but don't want to apply it in this case? Is it really cumbersome to do that in this CL, compared to some _actually_ complicated code in other pyauto hooks (that e.g. do a lot of checking of the input JSON dict - why don't you just CHECK there for consistency of your argument?). > In any case, I think this CL is now ready for commit. As said above, at least I'm really close to say "if you want to make your test more crashy I can't stop you", but I think that'd be bad for overall quality of the automation.
Thanks all for the comments. The patch has been refactored as per @jam's suggestion of not blocking the threads. Please have another look. @phajdan.jr: forwarding the return values of PostTask is not trivial in this version, so I've kept the CHECKs. wdyt? http://codereview.chromium.org/8274012/diff/25006/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/8274012/diff/25006/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:180: // BrowserThread::PostTask, and ignores the return value. I tried to resolve the overloading with bool (*post)(BrowserThread::ID, const Location&, const Closure&) = &BrowserThread::PostTask; And passing |post| to bind instead, but this had issues because the bool return value wasn't ignored. Adding base::IgnoreResult() then confused the nested Binds. At this point my humble c++ skills were defeated, and I opted for this. I also find it more readable in the end.
http://codereview.chromium.org/8274012/diff/25006/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/8274012/diff/25006/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:182: CHECK(BrowserThread::PostTask(id, FROM_HERE, task)); Please make it return the value it gets from the real PostTask, and let's see below if we still need to CHECK. We shouldn't. http://codereview.chromium.org/8274012/diff/25006/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:2747: if ((*iter)->handle() == base::kNullProcessHandle) { nit: No need for braces {}. http://codereview.chromium.org/8274012/diff/25006/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:2933: CHECK(BrowserThread::PostTaskAndReply( Why don't you just SendError if this fails? http://codereview.chromium.org/8274012/diff/25006/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:5906: PostTask(BrowserThread::IO, Please add some base::Bind gurus for possible advice about this. I'm sort of fine, but maybe there's a better way and we can all learn. http://codereview.chromium.org/8274012/diff/25006/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:5909: base::Bind(&PostTask, BrowserThread::UI, So it seems you've done the CHECK in PostTask because they're nested, but I think you can still pass the reply message as an argument and instead of CHECKing send an error message. Are there problems with that approach that I've missed?
thanks, this looks much better Why are you CHECKing the result of BrowserThread::PostTask? This will always succeed when the browser is running. The only time it might fail would be at shutdown, which shouldn't be happening here. And even then, a success return is meaningless because just the task got posted doesn't mean it will run.
@willchan: please have a look at the nested Binds. The purpose is to solve flakes in blacklist tests, since setting up the blacklist requires going to IO, then to FILE, then back to IO. Is there a better way to achieve this? @jam: makes sense, the CHECKs can be removed. @phajdan.jr: please see inline; wdyt? http://codereview.chromium.org/8274012/diff/25006/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/8274012/diff/25006/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:5906: PostTask(BrowserThread::IO, On 2011/10/21 15:13:17, Paweł Hajdan Jr. wrote: > Please add some base::Bind gurus for possible advice about this. I'm sort of > fine, but maybe there's a better way and we can all learn. Good idea, adding @willchan as the Bind guru :-) http://codereview.chromium.org/8274012/diff/25006/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:5909: base::Bind(&PostTask, BrowserThread::UI, On 2011/10/21 15:13:17, Paweł Hajdan Jr. wrote: > So it seems you've done the CHECK in PostTask because they're nested, but I > think you can still pass the reply message as an argument and instead of > CHECKing send an error message. Are there problems with that approach that I've > missed? I think replies have to be sent from UI; please correct me if I'm wrong about this. If so, then an inner PostTask failure would have to post SendError to UI, which can in turn fail again. I don't see what could be done in that case, and handling all that would complicate this a lot.
http://codereview.chromium.org/8274012/diff/25006/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/8274012/diff/25006/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:5906: PostTask(BrowserThread::IO, On 2011/10/21 16:25:53, Joao da Silva wrote: > On 2011/10/21 15:13:17, Paweł Hajdan Jr. wrote: > > Please add some base::Bind gurus for possible advice about this. I'm sort of > > fine, but maybe there's a better way and we can all learn. > > Good idea, adding @willchan as the Bind guru :-) This code gave me a good chuckle today. Thanks for sending it my way! I think what you've done is fine. That said, do you need to do UI=>IO=>FILE=>IO=>UI? That's not obvious from your comments. They make it sound like you just need to make sure you've done UI=>IO=>UI and UI=>FILE=>UI (which could be done in parallel). I can imagine this might be a more common task (indeed, we do something like this for shutdown to make sure we've flushed files to disk), and I can imagine creating a BarrierClosure or something to block until all the other closures have completed. I don't think the full sequence you've specified is common nor done in a cleaner way than what you already have. One thing to note though is that this approach is somewhat fragile. If someone updates URLBlacklistManager to have a different threading model, then won't obviously break, but may subtly break. If it becomes more common to do stuff like this, it may be advisable to create an explicit observer interface to watch for completion.
@willchan: thanks for taking a look! @phajdan.jr: I agree with John's remarks about checking PostTask's result, but I'd like to have your take on this before landing. http://codereview.chromium.org/8274012/diff/25006/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/8274012/diff/25006/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:2747: if ((*iter)->handle() == base::kNullProcessHandle) { On 2011/10/21 15:13:17, Paweł Hajdan Jr. wrote: > nit: No need for braces {}. Done. http://codereview.chromium.org/8274012/diff/25006/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:5906: PostTask(BrowserThread::IO, On 2011/10/21 20:50:29, willchan wrote: > On 2011/10/21 16:25:53, Joao da Silva wrote: > > On 2011/10/21 15:13:17, Paweł Hajdan Jr. wrote: > > > Please add some base::Bind gurus for possible advice about this. I'm sort of > > > fine, but maybe there's a better way and we can all learn. > > > > Good idea, adding @willchan as the Bind guru :-) > > This code gave me a good chuckle today. Thanks for sending it my way! I think > what you've done is fine. > > That said, do you need to do UI=>IO=>FILE=>IO=>UI? That's not obvious from your > comments. They make it sound like you just need to make sure you've done > UI=>IO=>UI and UI=>FILE=>UI (which could be done in parallel). I can imagine > this might be a more common task (indeed, we do something like this for shutdown > to make sure we've flushed files to disk), and I can imagine creating a > BarrierClosure or something to block until all the other closures have > completed. I don't think the full sequence you've specified is common nor done > in a cleaner way than what you already have. Thanks for taking a look. I've added an explicit comment about the update sequence of the blacklist. > > One thing to note though is that this approach is somewhat fragile. If someone > updates URLBlacklistManager to have a different threading model, then won't > obviously break, but may subtly break. If it becomes more common to do stuff > like this, it may be advisable to create an explicit observer interface to watch > for completion. This code could be moved to URLBlacklistManager::Update, with the last task replaced with a NotificationService::Notify(NOTIFICATION_URL_BLACKLIST_UPDATED) or similar. This place would then wait for that notification before replying, and changes to the URLBlacklistManager threading model would also change when the notification is fired. Since this is the only place needing such a notification, I'd leave as is for now. As an owner of policy/ and the blacklist in particular, I'll watch out for subtle changes :-)
http://codereview.chromium.org/8274012/diff/29001/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/8274012/diff/29001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:183: BrowserThread::PostTask(id, FROM_HERE, task) so i understand: we need this otherwise the bind templates will complain?
also lgtm
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/8274012/34002
@jam: see inline. Maybe Will knows a better way around this... http://codereview.chromium.org/8274012/diff/29001/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/8274012/diff/29001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:183: BrowserThread::PostTask(id, FROM_HERE, task) On 2011/10/25 05:42:10, John Abd-El-Malek wrote: > so i understand: we need this otherwise the bind templates will complain? Yes, I didn't manage to make it work otherwise. The first problem is that PostTask is overloaded, and Bind(&BrowserThread::PostTask, ...) will complain about not being able to resolve which one to call. I worked around this with bool (*post_task)(BrowserThread::ID, const Location&, const Closure&) = &BrowserThread::PostTask; Bind(post_task, ...); Then it hits a compile time assertion in base/callback.h "callback_type_does_not_match_bind_result". The bool return type of PostTask seemed to be the problem, so I wrapped the inner Binds with base::IgnoreReturn. Then it failed with "no matching function for call to ‘IgnoreReturn(... long list of templated stuff ...)'". At this point the nested binds were also quite unreadable, and I opted for this shortcut instead.
Change committed as 107113 |