Bug 175723

Summary: [Beacon] Improve error reporting
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit Misc.Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, darin, dbates, ggaren, japhet, mkwst, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 175443    
Bug Blocks: 147885    
Attachments:
Description Flags
WIP Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2017-08-18 10:18:07 PDT
Improve error reporting for beacon loads. Right now we only report synchronous errors, not asynchronous ones.
Comment 1 Chris Dumez 2017-08-18 13:32:16 PDT
Created attachment 318535 [details]
WIP Patch
Comment 2 Chris Dumez 2017-08-18 14:07:09 PDT
Created attachment 318538 [details]
Patch
Comment 3 Radar WebKit Bug Importer 2017-08-18 14:07:45 PDT
<rdar://problem/33969686>
Comment 4 Chris Dumez 2017-08-18 14:24:51 PDT
Created attachment 318541 [details]
Patch
Comment 5 Chris Dumez 2017-08-18 15:08:12 PDT
Created attachment 318549 [details]
Patch
Comment 6 Darin Adler 2017-08-18 15:40:07 PDT
Comment on attachment 318549 [details]
Patch

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

> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:49
> +    auto inflightBeacons = WTFMove(m_inflightBeacons);
> +    for (auto& beacon : inflightBeacons)
> +        beacon->removeClient(*this);

Is there really some risk of reentry here? If not, it seems unnecessary to move m_inflightBeacons into a local variable.

> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:54
> +    NavigatorBeacon* supplement = static_cast<NavigatorBeacon*>(Supplement<Navigator>::from(navigator, supplementName()));

Please consider auto* here.

> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:74
> +    bool wasRemoved = m_inflightBeacons.removeFirst(&resource);

If the operations we need are append and removeFirst, then normally we would want to use a Deque, not a Vector.

> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:92
> +    String description = error.localizedDescription();

Strange to use a "localized" description and then append hard-coded English text to it.

> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:95
> +            messageMiddle = ASCIILiteral(" due to access control checks.");

Why does this even compile? We are assigning to a const char*. Or we can have the local variable be of type ASCIILiteral, which I think would be better.

> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:100
> +    document->addConsoleMessage(MessageSource::Network, MessageLevel::Error, makeString("Beacon API cannot load ", error.failingURL().string(), messageMiddle, description));

We could do ASCIILiteral(messageMiddle) here.

> Source/WebCore/Modules/beacon/NavigatorBeacon.h:51
> +    static NavigatorBeacon* from(Navigator*);

Can this take a reference instead of a pointer?

> Source/WebCore/Modules/beacon/NavigatorBeacon.h:56
> +    // CachedRawResourceClient
> +    void notifyFinished(CachedResource&) final;
> +    void logError(const ResourceError&);

Seems unnecessary to comment which class we are overriding here. But especially strange to label this paragraph with the class name, but only one of the two functions here is an override.

> Source/WebCore/platform/network/PingHandle.h:48
> +        , m_timeoutTimer(*this, &PingHandle::timeoutTimerFired)

Consider using the lambda form of Timer instead of this one? I think we should use it in all new code, and only use the pointer-to-member-function one in legacy code.
Comment 7 Chris Dumez 2017-08-18 16:04:09 PDT
Comment on attachment 318549 [details]
Patch

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

>> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:49
>> +        beacon->removeClient(*this);
> 
> Is there really some risk of reentry here? If not, it seems unnecessary to move m_inflightBeacons into a local variable.

I don't think this is an issue currently. I'll get rid of the local.

>> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:54
>> +    NavigatorBeacon* supplement = static_cast<NavigatorBeacon*>(Supplement<Navigator>::from(navigator, supplementName()));
> 
> Please consider auto* here.

Ok.

>> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:74
>> +    bool wasRemoved = m_inflightBeacons.removeFirst(&resource);
> 
> If the operations we need are append and removeFirst, then normally we would want to use a Deque, not a Vector.

I think you may be confused by what removeFirst() does here. It removes the first match in the vector, not the first item in the vector.

>> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:95
>> +            messageMiddle = ASCIILiteral(" due to access control checks.");
> 
> Why does this even compile? We are assigning to a const char*. Or we can have the local variable be of type ASCIILiteral, which I think would be better.

Oops. I guess it works because of the "operator const char*() { return m_characters; }" operator on ASCIILiteral. Will fix.

>> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:100
>> +    document->addConsoleMessage(MessageSource::Network, MessageLevel::Error, makeString("Beacon API cannot load ", error.failingURL().string(), messageMiddle, description));
> 
> We could do ASCIILiteral(messageMiddle) here.

Ok.

>> Source/WebCore/Modules/beacon/NavigatorBeacon.h:51
>> +    static NavigatorBeacon* from(Navigator*);
> 
> Can this take a reference instead of a pointer?

Ok.

>> Source/WebCore/Modules/beacon/NavigatorBeacon.h:56
>> +    void logError(const ResourceError&);
> 
> Seems unnecessary to comment which class we are overriding here. But especially strange to label this paragraph with the class name, but only one of the two functions here is an override.

Ok.

>> Source/WebCore/platform/network/PingHandle.h:48
>> +        , m_timeoutTimer(*this, &PingHandle::timeoutTimerFired)
> 
> Consider using the lambda form of Timer instead of this one? I think we should use it in all new code, and only use the pointer-to-member-function one in legacy code.

This is not new code but I can take a look. Do you mean that we now prefer to inline the body of the timeoutTimerFired() method inside the init list?
Comment 8 Chris Dumez 2017-08-18 16:07:35 PDT
Comment on attachment 318549 [details]
Patch

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

>>> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:74
>>> +    bool wasRemoved = m_inflightBeacons.removeFirst(&resource);
>> 
>> If the operations we need are append and removeFirst, then normally we would want to use a Deque, not a Vector.
> 
> I think you may be confused by what removeFirst() does here. It removes the first match in the vector, not the first item in the vector.

Technically, this could be a HashSet. It seemed a little overkill at first but I guess a page could send a lot of beacons. Please let me know if you'd prefer a HashSet.
Comment 9 Chris Dumez 2017-08-18 16:42:01 PDT
Created attachment 318556 [details]
Patch
Comment 10 Chris Dumez 2017-08-18 16:43:09 PDT
(In reply to Chris Dumez from comment #8)
> Comment on attachment 318549 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318549&action=review
> 
> >>> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:74
> >>> +    bool wasRemoved = m_inflightBeacons.removeFirst(&resource);
> >> 
> >> If the operations we need are append and removeFirst, then normally we would want to use a Deque, not a Vector.
> > 
> > I think you may be confused by what removeFirst() does here. It removes the first match in the vector, not the first item in the vector.
> 
> Technically, this could be a HashSet. It seemed a little overkill at first
> but I guess a page could send a lot of beacons. Please let me know if you'd
> prefer a HashSet.

It seems we don't currently have the right traits to have a HashSet of CachedResourceHandles.
Comment 11 WebKit Commit Bot 2017-08-18 17:28:02 PDT
Comment on attachment 318556 [details]
Patch

Clearing flags on attachment: 318556

Committed r220946: <http://trac.webkit.org/changeset/220946>
Comment 12 WebKit Commit Bot 2017-08-18 17:28:04 PDT
All reviewed patches have been landed.  Closing bug.