Closed Bug 1416201 Opened 7 years ago Closed 7 years ago

HTTP request stack trace should be lazy loaded

Categories

(DevTools :: Netmonitor, enhancement, P2)

Product:
DevTools
Component:
Netmonitor
Type:
enhancement

Tracking

(firefox57 wontfix, firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox59 --- fixed

People

(Reporter: Honza, Assigned: Honza)

References

Details

Attachments

(3 files)

Bug 1416201 - HTTP request stack trace should be lazy loaded;
59 bytes, text/x-review-board-request
rickychien
: review+
Details
Bug 1416201 - update test fixtures for the Console panel;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details
Bug 1416201 - Fix tests;
59 bytes, text/x-review-board-request
rickychien
: review+
Details 
Similarly to how response content is lazy loaded (see bug 1404917) we should also lazy load stack-trace info. This info is generated mainly for XHR and is sent in the first `networkEvent` packet.

Every frame in the `stacktrace` field is represented by a structure describing the origin. The size of the structure tends to be big (especially associated URL) and also number of the frames can be significant.

The info shouldn't be send in the first packet, but requested explicitly, just like headers, cookies or response content. 

Here is an example of `networkEvent` packet(see `eventActor.cause.stacktrace` field):

{
  "from": "server1.conn1.child1\/consoleActor2",
  "type": "networkEvent",
  "eventActor": {
    "actor": "server1.conn1.child1\/netEvent41",
    "startedDateTime": "2017-11-10T12:11:28.511Z",
    "timeStamp": 1510315888511,
    "url": "http:\/\/10.0.3.111:8080\/www\/xhr-spy\/test.xml?a=test",
    "method": "POST",
    "isXHR": true,
    "cause": {
      "type": "xhr",
      "loadingDocumentUri": "http:\/\/10.0.3.111:8080\/www\/xhr-spy\/",
      "stacktrace": [
        {
          "filename": "http:\/\/10.0.3.111:8080\/www\/xhr-spy\/",
          "lineNumber": 143,
          "columnNumber": 5,
          "functionName": "onPostXml",
          "asyncCause": null
        },
        {
          "filename": "http:\/\/10.0.3.111:8080\/www\/xhr-spy\/",
          "lineNumber": 1,
          "columnNumber": 1,
          "functionName": "onclick",
          "asyncCause": null
        }
      ]
    },
    "private": false
  }
}

Honza
Blocks: 1350969
status-firefox57: --- → wontfix
Priority: -- → P2
This `stacktrace` field is set by the actor's codebase here:
  https://searchfox.org/mozilla-central/source/devtools/shared/webconsole/network-monitor.js#1767-1768

Just before calling console's actor onNetworkEvent:
  https://searchfox.org/mozilla-central/source/devtools/server/actors/webconsole.js#1620
Which emits the "networkEvent" that FirefoxDataProvider listen for.

So we could potentially stop setting this property and introduce a new RDP method in devtools/server/actors/webconsole.js in order to fetch it lazily via requestData.
The only complex thing is that we could still need to check on networkEvent message for cause.stacktrace in order to support old fennec/remote debugger versions.
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Comment on attachment 8931030 [details]
Bug 1416201 - HTTP request stack trace should be lazy loaded;

https://reviewboard.mozilla.org/r/202126/#review207638

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:683
(Diff revision 1)
> +  onStackTrace(response) {
> +    return this.updateRequest(response.from, {
> +      stacktrace: response.stacktrace
> +    }).then(() => {
> +      emit(EVENTS.RECEIVED_EVENT_STACKTRACE, response.from);
> +    });
> +  }

Let's convert promise to async/await and return `stacktrace` instance like responseConent [1].

[1] https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/connector/firefox-data-provider.js#652-662
Attachment #8931030 - Flags: review?(rchien) → review+
Comment on attachment 8931032 [details]
Bug 1416201 - Fix tests;

https://reviewboard.mozilla.org/r/202130/#review207640

LGTM. thanks!
Attachment #8931032 - Flags: review?(rchien) → review+
Comment on attachment 8931030 [details]
Bug 1416201 - HTTP request stack trace should be lazy loaded;

https://reviewboard.mozilla.org/r/202126/#review207638

> Let's convert promise to async/await and return `stacktrace` instance like responseConent [1].
> 
> [1] https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/connector/firefox-data-provider.js#652-662

oh, this part has been modified in fixed tests patch. Please ignore my comment. :)
Comment on attachment 8931030 [details]
Bug 1416201 - HTTP request stack trace should be lazy loaded;

https://reviewboard.mozilla.org/r/202126/#review207728

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:69
(Diff revision 1)
>          startedMillis: Date.parse(startedDateTime),
>          method,
>          url,
>          isXHR,
>          cause,
> +        stacktrace: cause.stacktrace,

I think this line is the key to support old backend, right?
You should comment here. Something like:
Compabibility code to support Firefox 58 and earlier that always send stacktrace immediately on networkEvent message.
FF59+ support fetching the traces lazily via requestData.

(The important things to mention is the firefox version where things changed, and be clear about what could be removed once the old versions are no longer supported)

::: devtools/server/actors/webconsole.js:2005
(Diff revision 1)
>     * network event.
>     *
>     * @param object networkEvent
>     *        The network event associated with this actor.
> +   * @param array stackTrace
> +   *        The stack trace associated with this network event.

This param doesn't exist.
(In reply to Alexandre Poirot [:ochameau] from comment #8)
> I think this line is the key to support old backend, right?
Correct

> You should comment here. Something like:
> Compabibility code to support Firefox 58 and earlier that always send
> stacktrace immediately on networkEvent message.
> FF59+ support fetching the traces lazily via requestData.
> 
> (The important things to mention is the firefox version where things
> changed, and be clear about what could be removed once the old versions are
> no longer supported)
Done

> ::: devtools/server/actors/webconsole.js:2005
> (Diff revision 1)
> >     * network event.
> >     *
> >     * @param object networkEvent
> >     *        The network event associated with this actor.
> > +   * @param array stackTrace
> > +   *        The stack trace associated with this network event.
> 
> This param doesn't exist.
Removed

Thanks Alex!
Honza
Comment on attachment 8931031 [details]
Bug 1416201 - update test fixtures for the Console panel;

https://reviewboard.mozilla.org/r/202128/#review207822

This is looking good , and will probably save time on packet encoding for large stacktrace :)

Out of curiosity, where do we get the stacktrace data from now ? Netmonitor redux state ?
Attachment #8931031 - Flags: review?(nchevobbe) → review+
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #14)
> Out of curiosity, where do we get the stacktrace data from now ? Netmonitor
> redux state ?
The stack-trace is stored in the same reducer 'requests' just requested lazily.

Thanks for the review Nicolas!
Honza
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6a93857a6568
HTTP request stack trace should be lazy loaded;r=rickychien
https://hg.mozilla.org/integration/autoland/rev/fb4dc6ac92a5
update test fixtures for the Console panel; r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/d81558109d84
Fix tests; r=rickychien
`WebConsoleClient.getStackTrace` method was also added into devtools-connection pacakage at:
https://github.com/devtools-html/devtools-core/pull/906
Honza
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: