Bug 159521 – Add always-on logging for downloads

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Keith Rollin]

Add Logging 2.0 support for downloads to be able to diagnose issues like <rdar://problem/26444360>

[Keith Rollin]

<rdar://problem/26630645>

[Keith Rollin]

Created attachment 283047 [details]
Patch

[Chris Dumez]

Comment on attachment 283047 [details]
Patch

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

> Source/WebKit2/ChangeLog:13
> +        Note that this logging only tracks WebKit-initiated downloading. It

TMI

[Chris Dumez]

Comment on attachment 283047 [details]
Patch

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

> Source/WebKit2/NetworkProcess/Downloads/Download.cpp:44
> +#if USE(NETWORK_SESSION) && PLATFORM(COCOA)

Do we really need this? What would prevent other non NetworkSession code to use those macros for e.g.?

> Source/WebKit2/NetworkProcess/Downloads/Download.cpp:95
> +#if USE(NETWORK_SESSION) && PLATFORM(COCOA)

Is this really needed?

> Source/WebKit2/NetworkProcess/Downloads/Download.cpp:104
> +#if USE(NETWORK_SESSION) && PLATFORM(COCOA)

Is this really needed?

> Source/WebKit2/NetworkProcess/Downloads/Download.cpp:146
> +#if USE(NETWORK_SESSION) && PLATFORM(COCOA)

Is this really needed?

> Source/WebKit2/NetworkProcess/Downloads/Download.cpp:164
> +#if USE(NETWORK_SESSION) && PLATFORM(COCOA)

Is this really needed?

> Source/WebKit2/NetworkProcess/Downloads/Download.cpp:180
> +#if USE(NETWORK_SESSION) && PLATFORM(COCOA)

Is this really needed?

> Source/WebKit2/NetworkProcess/Downloads/Download.cpp:203
> +#if USE(NETWORK_SESSION) && PLATFORM(COCOA)

Is this really needed?

[Keith Rollin]

(In reply to comment #4)
> Comment on attachment 283047 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283047&action=review
> 
> > Source/WebKit2/NetworkProcess/Downloads/Download.cpp:44
> > +#if USE(NETWORK_SESSION) && PLATFORM(COCOA)
> 
> Do we really need this? What would prevent other non NetworkSession code to
> use those macros for e.g.?

The logging macros reference isAlwaysOnLoggingAllowed(). That function looks at the associated sessionID to see if the session is private or not. That sessionID is available only in the "USE(NETWORK_SESSION) && PLATFORM(COCOA)" branches of code.

I suppose it could tweak things around another way. I could change isAlwaysOnLoggingAllowed to always return some hard-coded bool (true or false, it probably doesn't matter) rather than something that references m_sessionID in the non-"USE(NETWORK_SESSION) && PLATFORM(COCOA)" case.

[Chris Dumez]

Comment on attachment 283047 [details]
Patch

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

>>> Source/WebKit2/NetworkProcess/Downloads/Download.cpp:44
>>> +#if USE(NETWORK_SESSION) && PLATFORM(COCOA)
>> 
>> Do we really need this? What would prevent other non NetworkSession code to use those macros for e.g.?
> 
> The logging macros reference isAlwaysOnLoggingAllowed(). That function looks at the associated sessionID to see if the session is private or not. That sessionID is available only in the "USE(NETWORK_SESSION) && PLATFORM(COCOA)" branches of code.
> 
> I suppose it could tweak things around another way. I could change isAlwaysOnLoggingAllowed to always return some hard-coded bool (true or false, it probably doesn't matter) rather than something that references m_sessionID in the non-"USE(NETWORK_SESSION) && PLATFORM(COCOA)" case.

Or maybe have the DOWNLOAD_LOG_ALWAYS*() macros be no-ops for other platforms so that we don't need to #ifdef at every call site.

[Keith Rollin]

Created attachment 283062 [details]
Patch

[Keith Rollin]

Created attachment 283080 [details]
Patch

[Chris Dumez]

Comment on attachment 283080 [details]
Patch

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

This will require WK2 owner review but here is my informal review.

> Source/WebKit2/NetworkProcess/Downloads/Download.cpp:45
> +#define DOWNLOAD_LOG_ALWAYS_ERROR(...) LOG_ALWAYS_ERROR(isAlwaysOnLoggingAllowed(), __VA_ARGS__)

You're not using this one?

> Source/WebKit2/NetworkProcess/Downloads/Download.cpp:101
> +        DOWNLOAD_LOG_ALWAYS("Download task (%llu) received bytes", downloadID().downloadID());

Received *first* bytes ? Or "started receiving data" ?

> Source/WebKit2/NetworkProcess/Downloads/Download.cpp:102
> +        m_receivedData = true;

As per coding style, Boolean variables need a prefix. In this case, I suggest "hasReceivedData"

> Source/WebKit2/NetworkProcess/Downloads/Download.h:150
> +    WebCore::SessionID m_sessionID;

Shouldn't this be in the COCOA && NETWORK_SESSION block?

> Source/WebKit2/NetworkProcess/Downloads/Download.h:152
> +    bool m_receivedData = false;

I think we usually use the { false } syntax.

[Keith Rollin]

(In reply to comment #9)
> Comment on attachment 283080 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283080&action=review
> 
> This will require WK2 owner review but here is my informal review.
> 
> > Source/WebKit2/NetworkProcess/Downloads/Download.cpp:45
> > +#define DOWNLOAD_LOG_ALWAYS_ERROR(...) LOG_ALWAYS_ERROR(isAlwaysOnLoggingAllowed(), __VA_ARGS__)
> 
> You're not using this one?

Not this time. I've generally added them in pairs for consistency. And so people can assume that if one is there, so is the other.

> > Source/WebKit2/NetworkProcess/Downloads/Download.cpp:101
> > +        DOWNLOAD_LOG_ALWAYS("Download task (%llu) received bytes", downloadID().downloadID());
> 
> Received *first* bytes ? Or "started receiving data" ?

OK.

> > Source/WebKit2/NetworkProcess/Downloads/Download.cpp:102
> > +        m_receivedData = true;
> 
> As per coding style, Boolean variables need a prefix. In this case, I
> suggest "hasReceivedData"

OK.

> > Source/WebKit2/NetworkProcess/Downloads/Download.h:150
> > +    WebCore::SessionID m_sessionID;
> 
> Shouldn't this be in the COCOA && NETWORK_SESSION block?

You seemed to be encouraging the reduction of conditional compilation, so I didn't add a block here.

> > Source/WebKit2/NetworkProcess/Downloads/Download.h:152
> > +    bool m_receivedData = false;
> 
> I think we usually use the { false } syntax.

OK.

[Chris Dumez]

Comment on attachment 283080 [details]
Patch

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

>>> Source/WebKit2/NetworkProcess/Downloads/Download.h:150
>>> +    WebCore::SessionID m_sessionID;
>> 
>> Shouldn't this be in the COCOA && NETWORK_SESSION block?
> 
> You seemed to be encouraging the reduction of conditional compilation, so I didn't add a block here.

But... The block is already there above.

[Keith Rollin]

Yeah, but I didn't want to add to the list of things people had to think about. "Hrm. I don't remember...is m_sessionID available in this context or not?".

On the other hand, including an m_sessionID that is meaningful only some of the time is probably not a good idea, either.

Overall, I guess the best solution is to figure out how to get rid of these conditionals in the first place. :-) In the meantime, I'll put m_sessionID back up in the conditional block.

[Keith Rollin]

Created attachment 283092 [details]
Patch

[Alex Christensen]

Comment on attachment 283092 [details]
Patch

platform differences in networking and downloading code are going to be around for a long time.

[WebKit Commit Bot]

Comment on attachment 283092 [details]
Patch

Clearing flags on attachment: 283092

Committed r202993: <http://trac.webkit.org/changeset/202993>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.