Bug 52292 – Web Inspector: redirected resources not handled properly in Network panel

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

[Andrey Kosyakov]

There's a number of problems with handling redirected resources in network panel:
- redirected main resource is not shown (or shown improperly) in Network panel
- redirected resources are not present in HAR
- when page navigates away to a new main resource that returns redirect, the main resource is not updated

[Andrey Kosyakov]

Created attachment 78696 [details]
patch

[Pavel Feldman]

Comment on attachment 78696 [details]
patch

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

> Source/WebCore/inspector/InspectorInstrumentation.cpp:386
> +                                                   ic->isMainResourceLoader(loader, request.url()) && request.targetType() == ResourceRequestBase::TargetIsMainFrame);

Nit: please extract isMainResource.

> Source/WebCore/inspector/front-end/NetworkManager.js:35
> +    this._lastIdentifier = 0;

_lastIdentifierForCachedResource ?

> Source/WebCore/inspector/front-end/NetworkManager.js:282
> +        if (typeof identifier === "number")

This is a very dirty hack, please remove.

> Source/WebCore/inspector/front-end/NetworkPanel.js:770
> +    resourceById: function(id)

id does not make sense for resources loaded before front-end opening. Hence WebInspector.resourceById or NetworkManager::resourceById should not exist.

[Andrey Kosyakov]

Created attachment 78803 [details]
patch

[WebKit Review Bot]

Attachment 78803 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

Source/WebCore/inspector/InspectorController.h:313:  The parameter name "loader" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorController.h:313:  The parameter name "request" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pavel Feldman]

Comment on attachment 78803 [details]
patch

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

> Source/WebCore/inspector/front-end/ConsoleView.js:671
> +                var resource = this._requestId && WebInspector.panels.network.resourceById(this._requestId);

WebInspector.networkResourcesById()

> Source/WebCore/inspector/front-end/ExtensionServer.js:291
> +        resource = WebInspector.panels.network.resourceById(id) || WebInspector.resourceForURL(id);

I like the way it was more.

> Source/WebCore/inspector/front-end/ExtensionServer.js:321
> +        var resource = WebInspector.panels.network.resourceById(message.id);

ditto.

> Source/WebCore/inspector/front-end/NetworkManager.js:100
> +            WebInspector.panels.network.mainResourceChanged();

Updating main resource here and resetting network panel can't be done here.

> Source/WebCore/inspector/front-end/NetworkManager.js:220
> +        var resource = WebInspector.panels.network.resourceById(identifier);

ditto

> Source/WebCore/inspector/front-end/NetworkManager.js:305
> +        this._resourceTreeModel.bindResourceURL(resource);

You should be careful with bindResourceURL since unbind is automated and is only called for frame's resources.

[Andrey Kosyakov]

Created attachment 78821 [details]
patch

- Moved main resource detection from identifierForInitialRequest() back to didCommitLoad()
- Automatically bind resource to URL in ResourceTreeModel.createResource()
- Got rid of NetworkManager._createResource()
- Unbind redirected resources

Re WebInspector.networkResourcesById() and WebInspector.networkResources: the idea is that most consumers need a list and do not (or should not) care about resource ids, so I'm splitting the old WebInspector.networkResources "map" into an array and a method to retrieve specific resource by id. The latter is deprecated, sort of (it is currently used by extension server, which ultimately should maintain its own ids, and ConsoleView, which uses it to provide additional info for network errors).

[WebKit Review Bot]

Attachment 78821 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

Source/WebCore/inspector/InspectorController.h:313:  The parameter name "loader" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorController.h:313:  The parameter name "request" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andrey Kosyakov]

Created attachment 78823 [details]
patch

Removed InspectorController::isMainResource()

[Pavel Feldman]

Comment on attachment 78823 [details]
patch

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

> Source/WebCore/inspector/front-end/ConsoleView.js:671
> +                var resource = this._requestId && WebInspector.panels.network.resourceById(this._requestId);

Please fix this usage as well.

> Source/WebCore/inspector/front-end/NetworkManager.js:93
> +        this._resourcesById[identifier] = resource;

Nit: _resourcesById -> _resourcesBeingLoadedById (in flight)

> Source/WebCore/inspector/front-end/NetworkManager.js:294
> +        this._resourceTreeModel.unbindResourceURL(originalResource);

Nit: add FIXME: we should bind upon adding to the tree only (encapsulated into ResourceTreeModel), Script debugger should do explicit late binding on its own.

[Andrey Kosyakov]

Manually committed r75786: http://trac.webkit.org/changeset/75786

[WebKit Review Bot]

http://trac.webkit.org/changeset/75786 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
http/tests/inspector/extensions-resources-redirect.html