Bug 54729 – Web Inspector: flakyness of inspector tests

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Ilya Tikhonovsky]

many inspector tests are flaky.

I think the reason for this is a race between two pages.
We have inspected page and Inspector's frontend page. 
When frontend page is loaded it begins to interact with
Inspector's backend to gather information about the state
of the inspecting page. This process is completely asynchronous. 
A number of messages will be sent back and forth. 
At the same time the inspected page tries do its work and
send their messages to check the state inspector and trying
to do some things with it. However, the state of Inspector's page
is not stable until it finishes its initialization process. 

it would be better to wait until Inspector's frontend page will send
all their requests and receive back all the answers.
We definitely know how many requests are waiting for answers and need
just wait until the number becomes 0 and process test command after that.

[Ilya Tikhonovsky]

Created attachment 82931 [details]
[patch] initial version

[Alexander Pavlov (apavlov)]

Comment on attachment 82931 [details]
[patch] initial version

View in context: https://bugs.webkit.org/attachment.cgi?id=82931&action=review

> Source/WebCore/ChangeLog:6
> +

You should specify a bug URL after the issue title

> Source/WebCore/inspector/CodeGeneratorInspector.pm:629
> +        return !!this._waitForResponse;

The boolean cast is not necessary, and we normally do not use it (according to one of pfeldman's reviews)

[Pavel Feldman]

Comment on attachment 82931 [details]
[patch] initial version

We already have a counter for runAfterPendingDispatches.

[Ilya Tikhonovsky]

Created attachment 83293 [details]
[patch] initial version

now test harness:
1) doesn't use setTimeout for processing pending test scripts;
2) has no difference between WebKit and Chromium implementations;
3) eval pending test scripts only when all the responses are received.

[Pavel Feldman]

Comment on attachment 83293 [details]
[patch] initial version

View in context: https://bugs.webkit.org/attachment.cgi?id=83293&action=review

> Source/WebCore/inspector/CodeGeneratorInspector.pm:714
> +    this._waitForResponse = 0;

this._pendingResponseCount

> Source/WebCore/inspector/CodeGeneratorInspector.pm:732
> +        --this._waitForResponse;

move this line to after callback.apply

> Source/WebCore/inspector/CodeGeneratorInspector.pm:815
> +            if (this.runAfterPendingDispatches && WebInspector.pendingDispatches === 0 && !this.isWaitingForResponse())

pendingDispatches is of no use now.

> Source/WebCore/inspector/front-end/TestController.js:78
> +WebInspector.testController = new WebInspector.TestController();

You should not need it (leave as it was).

> Source/WebCore/inspector/front-end/TestController.js:79
> +WebInspector.evaluateForTestInFrontend = WebInspector.testController._evaluateForTestInFrontend.bind(WebInspector.testController);

Leave as it was.

[Ilya Tikhonovsky]

Created attachment 83301 [details]
[patch] third version

comments addressed

[Pavel Feldman]

Comment on attachment 83301 [details]
[patch] third version

View in context: https://bugs.webkit.org/attachment.cgi?id=83301&action=review

> Source/WebCore/inspector/CodeGeneratorInspector.pm:733
> +        --this._pendingResponsesCount;

you should move this into dispatch (after the if/else).

> Source/WebCore/inspector/CodeGeneratorInspector.pm:815
> +            if (this.runAfterPendingDispatches && !this.isWaitingForResponse())

Just move it here.

> Source/WebCore/inspector/front-end/TestController.js:77
> +    WebInspector.testController.runAfterPendingDispatches(invokeMethod);

I'd rather implement runAfterPendingDispatches on the InspectorBackendStub and use it both: from test controller and inspectortest.

[Ilya Tikhonovsky]

Committed r79322
	M	LayoutTests/http/tests/inspector/inspector-test.js
	M	LayoutTests/ChangeLog
	M	Source/WebKit/chromium/ChangeLog
	M	Source/WebKit/chromium/src/js/DevTools.js
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/inspector/CodeGeneratorInspector.pm
	M	Source/WebCore/inspector/front-end/inspector.js
	M	Source/WebCore/inspector/front-end/TestController.js
	D	Tools/DumpRenderTree/chromium/DRTDevToolsCallArgs.cpp
	D	Tools/DumpRenderTree/chromium/DRTDevToolsCallArgs.h
	M	Tools/DumpRenderTree/chromium/DRTDevToolsClient.cpp
	M	Tools/DumpRenderTree/chromium/DRTDevToolsAgent.h
	M	Tools/DumpRenderTree/chromium/DRTDevToolsClient.h
	M	Tools/DumpRenderTree/chromium/DRTDevToolsAgent.cpp
	M	Tools/DumpRenderTree/DumpRenderTree.gypi
	M	Tools/ChangeLog
W: -empty_dir: trunk/Tools/DumpRenderTree/chromium/DRTDevToolsCallArgs.cpp
W: -empty_dir: trunk/Tools/DumpRenderTree/chromium/DRTDevToolsCallArgs.h
r79322 = 169c0aa5e9b5d1327052690447638f5bcfa812cc (refs/remotes/trunk)

[Ilya Tikhonovsky]

Created attachment 83311 [details]
the patch I'm going to land.