[Devtools] Fixed bug with NetworkLog depending on ResourceTreeModel (issue 2546903002 by allada@chromium.org)

0 views
Skip to first unread message

all...@chromium.org

unread,
Dec 1, 2016, 6:17:24 PM12/1/16
to ca...@chromium.org, dgo...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org
Reviewers: caseq, dgozman
CL: https://codereview.chromium.org/2546903002/

Message:
PTL... I was going to write a test for this, but because it happens mostly for
service workers and service workers requires HTTPS and bots have limited support
for LayoutTests as HTTPS it's going to be a lot of work.

Description:
[Devtools] Fixed bug with NetworkLog depending on ResourceTreeModel

ResourceTreeModel is currently required by NetworkLog and causes a crash
when a NetworkRequest tries to access it's NetworkLog because it does not
exist. This bug fixes this dependency.

R=dgozman,caseq
BUG=668032

Affected files (+9, -6 lines):
M third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js
M third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js


Index: third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js
diff --git a/third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js b/third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js
index 176ae77911919a46e491bd57a32f2c3f62ef9869..ed62bf2e3437fdefdd08089564fc141892062c88 100644
--- a/third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js
+++ b/third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js
@@ -34,7 +34,7 @@
SDK.NetworkLog = class extends SDK.SDKModel {
/**
* @param {!SDK.Target} target
- * @param {!SDK.ResourceTreeModel} resourceTreeModel
+ * @param {?SDK.ResourceTreeModel} resourceTreeModel
* @param {!SDK.NetworkManager} networkManager
*/
constructor(target, resourceTreeModel, networkManager) {
@@ -43,10 +43,12 @@ SDK.NetworkLog = class extends SDK.SDKModel {
this._requests = [];
this._requestForId = {};
networkManager.addEventListener(SDK.NetworkManager.Events.RequestStarted, this._onRequestStarted, this);
- resourceTreeModel.addEventListener(
- SDK.ResourceTreeModel.Events.MainFrameNavigated, this._onMainFrameNavigated, this);
- resourceTreeModel.addEventListener(SDK.ResourceTreeModel.Events.Load, this._onLoad, this);
- resourceTreeModel.addEventListener(SDK.ResourceTreeModel.Events.DOMContentLoaded, this._onDOMContentLoaded, this);
+ if (resourceTreeModel) {
+ resourceTreeModel.addEventListener(
+ SDK.ResourceTreeModel.Events.MainFrameNavigated, this._onMainFrameNavigated, this);
+ resourceTreeModel.addEventListener(SDK.ResourceTreeModel.Events.Load, this._onLoad, this);
+ resourceTreeModel.addEventListener(SDK.ResourceTreeModel.Events.DOMContentLoaded, this._onDOMContentLoaded, this);
+ }
}

/**
Index: third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js
diff --git a/third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js b/third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js
index 09497a0d88bbf727dc145beb63db599c0d2db6e3..07f2a6a170f3ae84718c2e217ff063ce86b60768 100644
--- a/third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js
+++ b/third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js
@@ -176,8 +176,9 @@ SDK.TargetManager = class extends Common.Object {
if (networkManager && target.hasDOMCapability()) {
resourceTreeModel =
new SDK.ResourceTreeModel(target, networkManager, SDK.SecurityOriginManager.fromTarget(target));
- new SDK.NetworkLog(target, resourceTreeModel, networkManager);
}
+ if (networkManager)
+ new SDK.NetworkLog(target, resourceTreeModel, networkManager);

/** @type {!SDK.RuntimeModel} */
target.runtimeModel = new SDK.RuntimeModel(target);


dgo...@chromium.org

unread,
Dec 2, 2016, 1:08:23 PM12/2/16
to all...@chromium.org, ca...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org
Let's instead move page-load functionality to ResourceTreeModel?

Re: https in tests. localhost is considered to be a secure location, so http
LayoutTests perfectly work with serviceworkers.

https://codereview.chromium.org/2546903002/

pfel...@chromium.org

unread,
Dec 2, 2016, 2:48:29 PM12/2/16
to all...@chromium.org, ca...@chromium.org, dgo...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, kozyatins...@chromium.org
When/why do we have network log and not have resource tree model?

https://codereview.chromium.org/2546903002/

all...@chromium.org

unread,
Dec 2, 2016, 9:28:34 PM12/2/16
to ca...@chromium.org, dgo...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, kozyatins...@chromium.org
On 2016/12/02 19:48:28, pfeldman wrote:
> When/why do we have network log and not have resource tree model?

Because we end up having requests that are not apart of a NetworkLog.

see:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front_end/sdk/HAREntry.js?dr&q=harlog&sq=package:chromium&l=308

here we are explicitly expecting it to be present:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front_end/sdk/NetworkRequest.js?dr&q=networkrequest.&sq=package:chromium&l=47

We should always have a NetworkLog for requests, but requests may not be tied to
a ResourceTreeModel, since it may have been loaded from another target like
service worker which does have network capability but does not have a dom.
(also: node does not have a ResourceTreeModel and currently not network either).

https://codereview.chromium.org/2546903002/
Reply all
Reply to author
Forward
0 new messages