TestOverrideEncoding hanging is because TabProxy::WaitForNavigation can not g...

TestOverrideEncoding hanging is because TabProxy::WaitForNavigation can not get reply if current last last_navigation_time for tab _tracker is great than the input last_navigation_time.

See AutomationProvider::WaitForNavigation, it is handled by IPC_MESSAGE_HANDLER_DELAY_REPLY, which means the message handler need to send the reply message by itself.
According to current WaitForNavigation logic, if current last last_navigation_time for tab _tracker is less than the input last_navigation_time, the replay will be sent by NavigationNotificationObserver. Otherwise, the reply will never be sent.
So if somehow the test machine is slow, the navigation has happened before calling AutomationProvider::WaitForNavigation, the caller will never get reply. That is why TestOverrideEncoding hangs sometimes.

Please refer to http://chrome-svn/viewvc/chrome?view=rev&revision=9585 to see how the logic was changed before.

The problem also happened on other two places. 

BUG=23121
TEST=BrowserEncodingTest.TestOverrideEncoding

[Paweł Hajdan Jr., 2009-09-29 17:27:54]

LGTM, with one comment.

http://codereview.chromium.org/242024/diff/1/3
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/242024/diff/1/3#newcode1751
Line 1751: if (!wait_for_navigation || !success) {
This !wait_for_navigation || !success condition was there before, but looks
suspicious to me. It will send a failure message when success is true but
wait_for_navigation is false. I think that the reply param (currently
AUTOMATION_MSG_NAVIGATION_ERROR) should depend on success. If you think that
touching this may break too many things unexpectedly, please add a
TODO(phajdan.jr) to investigate it.

[ananta, 2009-09-29 17:33:01]

LGTM

[Johnny(Jianning) Ding, 2009-10-01 15:09:53]

Thanks, Paweł!
I have updated the CL. Please take another look.
I will land it later if you are OK with it.

http://codereview.chromium.org/242024/diff/1/3
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/242024/diff/1/3#newcode1751
Line 1751: if (!wait_for_navigation || !success) {
Yes, I agree with you that the reply value should depend on the success. So I
will only add a TODO here without changing  the code. I might also take a look
this TODO later.

On 2009/09/29 17:27:54, Paweł Hajdan Jr. wrote:
> This !wait_for_navigation || !success condition was there before, but looks
> suspicious to me. It will send a failure message when success is true but
> wait_for_navigation is false. I think that the reply param (currently
> AUTOMATION_MSG_NAVIGATION_ERROR) should depend on success. If you think that
> touching this may break too many things unexpectedly, please add a
> TODO(phajdan.jr) to investigate it.

[Paweł Hajdan Jr., 2009-10-01 22:35:47]

LGTM