Bug 36217 – Web Inspector: Inspector tests sometimes fail due to full paths being printed instead of relative paths

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

[Pavel Feldman]

inspector ...
inspector/console-dirxml.html -> failed
....
inspector/console-tests.html -> failed
.......
inspector/elements-panel-xhtml-structure.xhtml -> failed
.................

For some reason inspector is not being enabled now.

[Adam Roben (:aroben)]

(In reply to comment #0)
> inspector ...
> inspector/console-dirxml.html -> failed
> ....
> inspector/console-tests.html -> failed
> .......
> inspector/elements-panel-xhtml-structure.xhtml -> failed
> .................
> 
> For some reason inspector is not being enabled now.

It doesn't seem that it isn't enabled. It seems that it's not stripping the URLs correctly.

[Adam Roben (:aroben)]

I think r56095 is a red herring. Before that revision, so many tests were failing that the Windows bots were aborting the tests early. I think it's far more likely that these tests were failing before r56095, but they just weren't being run.

[Pavel Feldman]

Clarification: the reason console messages have short urls is that there is a
resource entry with matching url. We trim long ones and use friendly shortcuts
in case there is matching resource entry. In this case, corresponding resource
entry is likely to be missing. Either resource tracking is disabled or resource
is not being reported as processed to the loader clients.

[Adam Roben (:aroben)]

(In reply to comment #3)
> Clarification: the reason console messages have short urls is that there is a
> resource entry with matching url. We trim long ones and use friendly shortcuts
> in case there is matching resource entry. In this case, corresponding resource
> entry is likely to be missing. Either resource tracking is disabled or resource
> is not being reported as processed to the loader clients.

Another possibility is that the URLs of the messages and the URLs of the resources differ in some way. E.g., maybe one is canonicalized and the other isn't.

[Adam Roben (:aroben)]

(In reply to comment #4)
> Another possibility is that the URLs of the messages and the URLs of the
> resources differ in some way. E.g., maybe one is canonicalized and the other
> isn't.

A little debugging shows that this doesn't seem to be the issue. I see the console-dirxml.html resource being added, and the console messages seem to have the same URL that the resource had.

[Adam Roben (:aroben)]

<rdar://problem/7827667>

[Adam Roben (:aroben)]

Ilya is looking into this. Our current theory is that, in Release builds, developer extras aren't getting enabled until after the main resource has already loaded, meaning that the main resources isn't in the Inspector's resource map.

[Ilya Tikhonovsky]

I've made additional round of investigation:
Usually inspector tests are failing under stress conditions generated by CPU and Disk I/O intensive tasks.

In normal situation the sequence of test events is pretty clear.
1) WebInspector: loaded
2) new resource: file:///E:/chromium/src/third_party/WebKit/LayoutTests/inspector/console-dirxml.html
3) Request for display name of url: file:///E:/chromium/src/third_party/WebKit/LayoutTests/inspector/console-dirxml.html

When some test is failed then typical sequence looks like the next one:
1) WebInspector: loaded
2) new resource: file:///E:/chromium/src/third_party/WebKit/LayoutTests/inspector/console-dirxml.html
3) new resource: file:///E:/chromium/src/third_party/WebKit/LayoutTests/inspector/console-format-collections.html
4) WebInspector: reset 
5) new resource: undefined 
6) Request for url: file:///E:/chromium/src/third_party/WebKit/LayoutTests/inspector/console-format-collections.html

WebInspector is closing asynchronously and that is the reason. 
Under stress conditions the next can accidentally use WebInspector of previous test.
I'll try to fix that today/tomorrow.

[Adam Roben (:aroben)]

Thanks for the analysis, Ilya!

[Ilya Tikhonovsky]

Created attachment 53039 [details]
[patch] Just first version of patch to check it on the try bots.

[Pavel Feldman]

Comment on attachment 53039 [details]
[patch] Just first version of patch to check it on the try bots.

> -void InspectorController::disconnectFrontend()
> +void InspectorController::disconnectFrontend(InspectorFrontend* frontend)
>  {
> -    if (!m_frontend)
> +    if (m_frontend != frontend)
>          return;

I think we should cut this control flow earlier. I.e. when connection is terminated upon backend request, there should be no further disconnect calls re-entering backend from within front-end. I think we've discussed it. Leaving it your way would make design less clean and make socket implementation tough.

[WebKit Review Bot]

Attachment 53039 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1590418

[Ilya Tikhonovsky]

pfeldman: we have four different ways to close inspector.
1) by LayoutTestController;
2) by navigation to other domain;
3) by close button of inspector when it is in attached state;
4) by native window button when it is in detached state;

We have problem with async close only in the first case.
and we should be able to do that when disconnect is initiated by inspector itself.

[Ilya Tikhonovsky]

Created attachment 53083 [details]
[patch] second iteration.

[Pavel Feldman]

(In reply to comment #13)
> pfeldman: we have four different ways to close inspector.
> 1) by LayoutTestController;
> 2) by navigation to other domain;
> 3) by close button of inspector when it is in attached state;
> 4) by native window button when it is in detached state;
> 
> We have problem with async close only in the first case.
> and we should be able to do that when disconnect is initiated by inspector
> itself.

I'd like to see more clarity in the code still. We have two parties (inspector on one side and front-end on another side) with serialized asynchronous connection in between. We need to have a proper connection handshake / termination routines. This connection can be terminated by either of the parties. I'd like to see clear methods for closing the connection initiated by inspector and the front-end. And once connection has been terminated, I don't expect echo methods to be called upon closed connection. In you code I see:

Inspector initiates disconnect -> sends message to frontend and cuts the pipe -> front-end initiates disconnect. And the reason you don't re-enter code on front-end is that the pipe is already cut. This sounds confusing. Front-end should only initiate disconnect when its host or close button or whatever on its side initiates disconnect.

[Pavel Feldman]

Comment on attachment 53083 [details]
[patch] second iteration.

(I don't see how my comments are addressed in this patch).

[Yury Semikhatsky]

(In reply to comment #15)
> Front-end
> should only initiate disconnect when its host or close button or whatever on
> its side initiates disconnect.
If WebInspector.close is asynchronous, there is another possible scenario:
1. The IC->FE pointer is cleared
2. WebInspector.close is called 
3. FE window close is handled and IC->disconnectFE is called.
At the third step it's possible that IC has already open another FE. This means that every FE client should track whether it was closed from the IC or not.

[Pavel Feldman]

(In reply to comment #17)
> (In reply to comment #15)
> > Front-end
> > should only initiate disconnect when its host or close button or whatever on
> > its side initiates disconnect.
> If WebInspector.close is asynchronous, there is another possible scenario:
> 1. The IC->FE pointer is cleared

At this point we should also clear the pointer from the transport implementation to inspector controller on the inspector page side, so that no more incoming messages are coming down this pipe.

> 2. WebInspector.close is called 
> 3. FE window close is handled and IC->disconnectFE is called.
> At the third step it's possible that IC has already open another FE. This means
> that every FE client should track whether it was closed from the IC or not.

[Yury Semikhatsky]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #15)
> > > Front-end
> > > should only initiate disconnect when its host or close button or whatever on
> > > its side initiates disconnect.
> > If WebInspector.close is asynchronous, there is another possible scenario:
> > 1. The IC->FE pointer is cleared
> 
> At this point we should also clear the pointer from the transport
> implementation to inspector controller on the inspector page side, so that no
> more incoming messages are coming down this pipe.
> 
We can easily do this with remote frontend but in case of in-process FE we need FE client to handle this.

[Ilya Tikhonovsky]

Created attachment 53352 [details]
[patch] next iteration

WebInspector.updateResource wants to have url in the each update payload because that property will be used if resource map has no such request.

[Pavel Feldman]

Comment on attachment 53352 [details]
[patch] next iteration

Hm. How do we end up in a state where we have resource with given id in the front-end, but it has lost its url? This should not be possible.

[Ilya Tikhonovsky]

Created attachment 53367 [details]
[patch] just fix.

[Ilya Tikhonovsky]

User scenario

0) run release webkit on windows
1) open inspector
2) enable resource tracking
3) close inspector
4) print yahoo.com into address bar
5) press enter and then immediately open inspector by Ctrl-Shift-I

Expected: resource pane should have a list of loaded resources with their names etc.
Actual: the first resource has no name, content and other attributes.

[Adam Roben (:aroben)]

*** Bug 36946 has been marked as a duplicate of this bug. ***

[Adam Roben (:aroben)]

Dupe bug 36946 says that this affects Tiger as well.

[Eric Seidel (no email)]

This is one of the more common failures these days.  Hit it again just now:
http://build.webkit.org/results/Tiger%20Intel%20Release/r58084%20(11119)/inspector/console-log-before-inspector-open-pretty-diff.html

Glad to see we have a fix posted.  Any chance of review?

[Eric Seidel (no email)]

I know next-to-nothing about the inspector or I would be happy to review it myself.

[Adam Roben (:aroben)]

(In reply to comment #26)
> This is one of the more common failures these days.  Hit it again just now:
> http://build.webkit.org/results/Tiger%20Intel%20Release/r58084%20(11119)/inspector/console-log-before-inspector-open-pretty-diff.html
> 
> Glad to see we have a fix posted.  Any chance of review?

Last I heard, Pavel doesn't think this fix will work. I'm not sure why he hasn't r-ed it yet.

[WebKit Commit Bot]

Comment on attachment 53367 [details]
[patch] just fix.

Clearing flags on attachment: 53367

Committed r58162: <http://trac.webkit.org/changeset/58162>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Roben (:aroben)]

Pavel, can you comment on whether you think attachment 53367 [details] (landed in r58162) was the right fix for this issue? Last I heard, you had issues with it.

[Adam Roben (:aroben)]

(In reply to comment #31)
> Pavel, can you comment on whether you think attachment 53367 [details] (landed in r58162)
> was the right fix for this issue? Last I heard, you had issues with it.

Pavel said on IRC:

"I was wrong about the patch. loislo did the right fix. I was too sure async close was to blame. We should still fix it, bug it gets lower priority."