Bug 150742 - Share more code between NETWORK_SESSION and non-NETWORK_SESSION NetworkResourceLoaders
Summary: Share more code between NETWORK_SESSION and non-NETWORK_SESSION NetworkResour...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks: 150834
  Show dependency treegraph
 
Reported: 2015-10-30 16:55 PDT by Alex Christensen
Modified: 2015-11-02 21:59 PST (History)
1 user (show)

See Also:


Attachments
Patch (19.02 KB, patch)
2015-10-30 17:00 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (19.04 KB, patch)
2015-10-30 17:14 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (21.73 KB, patch)
2015-10-30 17:49 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (13.23 KB, patch)
2015-10-31 12:07 PDT, Alex Christensen
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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