Bug 150742

Summary: Share more code between NETWORK_SESSION and non-NETWORK_SESSION NetworkResourceLoaders
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: koivisto
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 150834    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch darin: review+

Description Alex Christensen 2015-10-30 16:55:52 PDT
Share more code between NETWORK_SESSION and non-NETWORK_SESSION WebResourceLoaders
Comment 1 Alex Christensen 2015-10-30 17:00:54 PDT
Created attachment 264443 [details]
Patch
Comment 2 Alex Christensen 2015-10-30 17:14:23 PDT
Created attachment 264446 [details]
Patch
Comment 3 Alex Christensen 2015-10-30 17:49:00 PDT
Created attachment 264449 [details]
Patch
Comment 4 Alex Christensen 2015-10-31 12:07:30 PDT
Created attachment 264482 [details]
Patch
Comment 5 Darin Adler 2015-10-31 14:36:29 PDT
Comment on attachment 264482 [details]
Patch

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

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:497
> +    // The NetworkProcess should never get a didReceiveData callback.
> +    // We should always be using didReceiveBuffer.
> +    ASSERT_NOT_REACHED();

This kind of thing usually indicates a C++ object design mistake.

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:522
> +    ASSERT_UNUSED(handle, handle == m_handle);
> +
> +    sharedWillSendRedirectedRequest(request, redirectResponse);

Strange that there’s a blank line here, but not in all the other functions.

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.h:142
> +    void sharedDidReceiveBuffer(PassRefPtr<WebCore::SharedBuffer>, int reportedEncodedDataLength);

New code should not use PassRefPtr. It should use SharedBuffer&, or SharedBuffer*, or Ref<SharedBuffer>&&, or RefPtr<SharedBuffer>&& depending on whether the buffer can be null and whether ownership is being transferred or not.
Comment 6 Alex Christensen 2015-10-31 17:12:48 PDT
(In reply to comment #5)
> Comment on attachment 264482 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264482&action=review
> 
> > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:497
> > +    // The NetworkProcess should never get a didReceiveData callback.
> > +    // We should always be using didReceiveBuffer.
> > +    ASSERT_NOT_REACHED();
> 
> This kind of thing usually indicates a C++ object design mistake.
I agree.  Cleaning this up is outside of the scope of this patch, though.
> 
> > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:522
> > +    ASSERT_UNUSED(handle, handle == m_handle);
> > +
> > +    sharedWillSendRedirectedRequest(request, redirectResponse);
> 
> Strange that there’s a blank line here, but not in all the other functions.
> 
> > Source/WebKit2/NetworkProcess/NetworkResourceLoader.h:142
> > +    void sharedDidReceiveBuffer(PassRefPtr<WebCore::SharedBuffer>, int reportedEncodedDataLength);
> 
> New code should not use PassRefPtr. It should use SharedBuffer&, or
> SharedBuffer*, or Ref<SharedBuffer>&&, or RefPtr<SharedBuffer>&& depending
> on whether the buffer can be null and whether ownership is being transferred
> or not.
RefPtr<SharedBuffer>&& it is, then.
Comment 7 Alex Christensen 2015-10-31 17:15:56 PDT
http://trac.webkit.org/changeset/191848
Comment 8 Darin Adler 2015-10-31 17:50:53 PDT
Comment on attachment 264482 [details]
Patch

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

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:473
> +    sharedDidReceiveBuffer(buffer, buffer->size());

The patch that was landed changed this to:

    sharedDidReceiveBuffer(WTF::move(buffer), buffer->size());

That’s wrong, because passing the first argument can modify buffer, changing it to be null, and the passing the second argument evaluates buffer. There is no guarantee of the order of evaluation of arguments, so this needs to be written out as two lines:

    auto size = buffer->size();
    sharedDidReceiveBuffer(WTF::move(buffer), size);

Please fix that.
Comment 9 Alex Christensen 2015-11-02 17:51:22 PST
(In reply to comment #8)
> Please fix that.
Followup in https://bugs.webkit.org/show_bug.cgi?id=150829