Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1515183003:
Add a crash course to debugging using about:net-internals. (Closed)
DescriptionAdd a crash course to debugging using about:net-internals.
BUG=none
Committed: https://crrev.com/429beb6c2e66283a351ad9e2423dfee8afa86257
Cr-Commit-Position: refs/heads/master@{#365142}
Patch Set 1 #
Total comments: 24
Patch Set 2 : Response to comments #
Messages
Total messages: 15 (7 generated)
Description was changed from ========== Add an overview to debugging using about:net-internals. BUG=none ========== to ========== Add a crash course to debugging using about:net-internals. BUG=none ==========
mmenke@chromium.org changed reviewers: + rdsmith@chromium.org
Randy: WDYT? This isn't intended to be comprehensive, just trying to give some hints to get started actually debugging, and give some quick information a couple common scenarios. Just targeted at nooglers. Hoping I'll flesh it out as I debug issues and realize other things I should have included.
On 2015/12/11 21:04:43, mmenke wrote: > Randy: WDYT? This isn't intended to be comprehensive, just trying to give some > hints to get started actually debugging, and give some quick information a > couple common scenarios. Just targeted at nooglers. Hoping I'll flesh it out > as I debug issues and realize other things I should have included. And FYI, I'm thinking I'll spend some time trying to improve the triage docs next week (More finch info, rework needs-feedback section to be more aggressive, cover all sublabels, be less aggressive on filing bugs for infrequent crashers, more aggressive on following up on issues, make triage emails an official part of the process, etc).
LGTM (with documentation, any documentation is usually better than no documentation, and it's not clear what standard to aim for, so what's below are all suggestions). https://codereview.chromium.org/1515183003/diff/1/net/docs/crash-course-in-ne... File net/docs/crash-course-in-net-internals.md (right): https://codereview.chromium.org/1515183003/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:8: It would probably be useful to read "Life of a URLRequest" before this document. Probably useful to include a link. https://codereview.chromium.org/1515183003/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:27: show objects that were created before about:net-internals was openned. Since nit: opened. https://codereview.chromium.org/1515183003/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:30: profile's main URLRequestContext. Most debugging is done with the Events tab Suggestion: Collapse the distinction between "a particular URLRequestContext" and "the currently active profile's main URLRequestContext"; just make the first sentence "are all snapshots of the current state of only the active profile's main URLRequestContext, and are updated on a 5 second timer." https://codereview.chromium.org/1515183003/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:38: lifetime. Some events have a beginning and end point (During which other nit, possible style conflict: What I remember being taught about capitalization of phrases is that you don't use an initiial capital for phrases in parenthesis unless they are complete sentences (in which case they also have a period). You clearly have a different axiom, but I thought I'd raise the question in case it's reflex rather than choice. (I did some Googling to try and find a reference on this, but found too many, none of which I found authoritative, so I let that go.) https://codereview.chromium.org/1515183003/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:46: HTTP2 [SPDY]/QUIC) and what they include: nit, suggestion: move parenthetical to just before ":" https://codereview.chromium.org/1515183003/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:51: HttpStreamParsers used to service a response. If we follow HTTP redirects, it nit, suggestion: Should we (:-}) be using "we" in docs? I'm not sure whether or not to extend the comment anti-pattern to docs. https://codereview.chromium.org/1515183003/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:55: response). tangent: This is actually a frustration to me--I've sometimes wanted to see the headers when the cache handles the response. https://codereview.chromium.org/1515183003/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:57: * HTTP_STREAM_JOB: This corresponds to HttpStreamFactoryImpl::Job (Note that Why isn't this part of the URL_REQUEST source? It would seem like it's always on behalf of a single URL_REQUEST, and the rest of the URL_REQUEST isn't doing anything while it's active . Your judgement as to whether the answer deserves to be in the doc; maybe it's just historical. It just struck me funny on second reading. https://codereview.chromium.org/1515183003/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:59: lookups. One of the final event of these indicate how they created an nit: events. I also find "of these" to be awkward; maybe "One of the final events of this source indicates how it created an HttpStream."? https://codereview.chromium.org/1515183003/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:111: down (or up) into other related sources. This sorta suggests you can drill up from URL_REQUEST (because it comes after the doc talks about starting at URL_REQUEST). Dunno if that's worth clarifying. https://codereview.chromium.org/1515183003/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:114: event, and try to deduce what went wrong before/after a particular event. I've found this tricky because the default emacs grep arguments include "search for whole words only", so, for me, being clear here that the source code string contains but does not equal the event name is important. But I have no idea how many people are in the same position I am. https://codereview.chromium.org/1515183003/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:121: fun and exciting effects on underway network activity. Suggestion: Include guidance on how you can tell from the net-internals dump that this has happened?
mmenke@chromium.org changed reviewers: + eroman@chromium.org
[+eroman]: Thoughts? This was largely motivated by a Noogler's confusion over what to do during triage. Plan to try to improve the triage suggestion docs as well. https://codereview.chromium.org/1515183003/diff/1/net/docs/crash-course-in-ne... File net/docs/crash-course-in-net-internals.md (right): https://codereview.chromium.org/1515183003/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:8: It would probably be useful to read "Life of a URLRequest" before this document. On 2015/12/14 02:34:25, rdsmith wrote: > Probably useful to include a link. Done. https://codereview.chromium.org/1515183003/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:27: show objects that were created before about:net-internals was openned. Since On 2015/12/14 02:34:26, rdsmith wrote: > nit: opened. Done. https://codereview.chromium.org/1515183003/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:30: profile's main URLRequestContext. Most debugging is done with the Events tab On 2015/12/14 02:34:25, rdsmith wrote: > Suggestion: Collapse the distinction between "a particular URLRequestContext" > and "the currently active profile's main URLRequestContext"; just make the first > sentence "are all snapshots of the current state of only the active profile's > main URLRequestContext, and are updated on a 5 second timer." Done, removed second sentence. https://codereview.chromium.org/1515183003/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:38: lifetime. Some events have a beginning and end point (During which other On 2015/12/14 02:34:25, rdsmith wrote: > nit, possible style conflict: What I remember being taught about capitalization > of phrases is that you don't use an initiial capital for phrases in parenthesis > unless they are complete sentences (in which case they also have a period). You > clearly have a different axiom, but I thought I'd raise the question in case > it's reflex rather than choice. (I did some Googling to try and find a > reference on this, but found too many, none of which I found authoritative, so I > let that go.) You're right about the rule. I use a lot of parentheses, and always incorrectly capitalize the first letter after, even knowing the style is wrong. I've tried to fix up my style a bit. Periods also go inside the end of a parenthetical comment at the end of the comment as well, but that's too bizarre for me to do - it violates C syntax ordering rules, dangit! https://codereview.chromium.org/1515183003/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:46: HTTP2 [SPDY]/QUIC) and what they include: On 2015/12/14 02:34:26, rdsmith wrote: > nit, suggestion: move parenthetical to just before ":" Done. https://codereview.chromium.org/1515183003/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:51: HttpStreamParsers used to service a response. If we follow HTTP redirects, it On 2015/12/14 02:34:25, rdsmith wrote: > nit, suggestion: Should we (:-}) be using "we" in docs? I'm not sure whether or > not to extend the comment anti-pattern to docs. Done. https://codereview.chromium.org/1515183003/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:55: response). On 2015/12/14 02:34:26, rdsmith wrote: > tangent: This is actually a frustration to me--I've sometimes wanted to see the > headers when the cache handles the response. Patches welcome! I agree that we should improve things here - I'd also really like the cache to log more of its revalidation logic. https://codereview.chromium.org/1515183003/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:57: * HTTP_STREAM_JOB: This corresponds to HttpStreamFactoryImpl::Job (Note that On 2015/12/14 02:34:25, rdsmith wrote: > Why isn't this part of the URL_REQUEST source? It would seem like it's always > on behalf of a single URL_REQUEST, and the rest of the URL_REQUEST isn't doing > anything while it's active . > > Your judgement as to whether the answer deserves to be in the doc; maybe it's > just historical. It just struck me funny on second reading. Because we may create two of them (One for QUIC). Added that comment. https://codereview.chromium.org/1515183003/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:59: lookups. One of the final event of these indicate how they created an On 2015/12/14 02:34:26, rdsmith wrote: > nit: events. I also find "of these" to be awkward; maybe "One of the final > events of this source indicates how it created an HttpStream."? Done. Also fixed SpdySession / QuicSession to use the actual source names. https://codereview.chromium.org/1515183003/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:111: down (or up) into other related sources. On 2015/12/14 02:34:26, rdsmith wrote: > This sorta suggests you can drill up from URL_REQUEST (because it comes after > the doc talks about starting at URL_REQUEST). Dunno if that's worth clarifying. Done. https://codereview.chromium.org/1515183003/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:114: event, and try to deduce what went wrong before/after a particular event. On 2015/12/14 02:34:26, rdsmith wrote: > I've found this tricky because the default emacs grep arguments include "search > for whole words only", so, for me, being clear here that the source code string > contains but does not equal the event name is important. But I have no idea how > many people are in the same position I am. I use codesearch and MSVC, neither of which does that by default, so problem hadn't occurred to me. Added comment. https://codereview.chromium.org/1515183003/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:121: fun and exciting effects on underway network activity. On 2015/12/14 02:34:25, rdsmith wrote: > Suggestion: Include guidance on how you can tell from the net-internals dump > that this has happened? Added comment.
lgtm!
The CQ bit was checked by mmenke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/1515183003/#ps20001 (title: "Response to comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1515183003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1515183003/20001
Message was sent while issue was closed.
Description was changed from ========== Add a crash course to debugging using about:net-internals. BUG=none ========== to ========== Add a crash course to debugging using about:net-internals. BUG=none ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add a crash course to debugging using about:net-internals. BUG=none ========== to ========== Add a crash course to debugging using about:net-internals. BUG=none Committed: https://crrev.com/429beb6c2e66283a351ad9e2423dfee8afa86257 Cr-Commit-Position: refs/heads/master@{#365142} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/429beb6c2e66283a351ad9e2423dfee8afa86257 Cr-Commit-Position: refs/heads/master@{#365142} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
