Bug 112103 – Loads are never canceled in the NetworkProcess

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brady Eidson]

Loads are never canceled in the NetworkProcess

If a connection to a WebProcess goes away, then all of its scheduled loads should never be started, and all of its in-progress loads should be cancelled.

In radar as <rdar://problem/12890500>

[Brady Eidson]

Created attachment 192641 [details]
Patch v1

[Alexey Proskuryakov]

Comment on attachment 192641 [details]
Patch v1

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

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:110
> +        callOnMainThread(NetworkResourceLoader::performCleanups, 0);

We now have a version of callOnMainThread in WebCore that is type safe, or (as helpful here) doesn't require passing an argument.

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:157
> +#ifndef NDEBUG

Should probably check ASSERTIONS_ENABLED, not NDEBUG.

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:173
> +template<typename U> void NetworkResourceLoader::sendAbortingOnFailure(const U& message)
> +{
> +    if (!send(message))
> +        abortInProgressLoad();

Maybe it's OK, but the name made me wonder if an actual abort() will be called.

Given that sendSync doesn't get this treatment, I'd consider just doing this inline for consistency.

> Source/WebKit2/NetworkProcess/SyncNetworkResourceLoader.h:45
> +    virtual void connectionToWebProcessDidClose();

OVERRIDE

> Source/WebKit2/NetworkProcess/SyncNetworkResourceLoader.h:47
>      virtual bool isSynchronous() { return true; }

Yes, here too.

[Brady Eidson]

(In reply to comment #2)
> (From update of attachment 192641 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192641&action=review
> 
> > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:110
> > +        callOnMainThread(NetworkResourceLoader::performCleanups, 0);
> 
> We now have a version of callOnMainThread in WebCore that is type safe, or (as helpful here) doesn't require passing an argument.

callOnMainThread is a WTF:: concept, and the only other form I see takes a Function object.  I could certainly use that here but it just uses the function ptr form behind the scenes...

> > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:157
> > +#ifndef NDEBUG
> 
> Should probably check ASSERTIONS_ENABLED, not NDEBUG.

ASSERTIONS_ENABLED doesn't match anything in any of the projects.  Do you mean something else?

> > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:173
> > +template<typename U> void NetworkResourceLoader::sendAbortingOnFailure(const U& message)
> > +{
> > +    if (!send(message))
> > +        abortInProgressLoad();
> 
> Maybe it's OK, but the name made me wonder if an actual abort() will be called.
> 
> Given that sendSync doesn't get this treatment, I'd consider just doing this inline for consistency.

abortInProgressLoad() originally had one more line than it does, and that level of code copying drove me nuts.

I like having the abort code encapsulated, so I'll take the alternate form of your suggestion and give sendSync the same treatment I gave send.

> > Source/WebKit2/NetworkProcess/SyncNetworkResourceLoader.h:45
> > +    virtual void connectionToWebProcessDidClose();
> 
> OVERRIDE
> 
> > Source/WebKit2/NetworkProcess/SyncNetworkResourceLoader.h:47
> >      virtual bool isSynchronous() { return true; }
> 
> Yes, here too.

Willdo on both of these!

[Brady Eidson]

(In reply to comment #3)
> (In reply to comment #2)

> > > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:157
> > > +#ifndef NDEBUG
> > 
> > Should probably check ASSERTIONS_ENABLED, not NDEBUG.
> 
> ASSERTIONS_ENABLED doesn't match anything in any of the projects.  Do you mean something else?

You almost certainly meant #if !ASSERT_DISABLED

[Alexey Proskuryakov]

> callOnMainThread is a WTF:: concept, and the only other form I see takes a Function object.  I could certainly use that here but it just uses the function ptr form behind the scenes...

I'm talking about the one in WebCore/platform/MainThreadTask.h. It certainly uses the WTF one behind the scenes, but doesn't force you to pass a fake void* argument.

[Brady Eidson]

http://trac.webkit.org/changeset/145485

[Brady Eidson]

(In reply to comment #5)
> > callOnMainThread is a WTF:: concept, and the only other form I see takes a Function object.  I could certainly use that here but it just uses the function ptr form behind the scenes...
> 
> I'm talking about the one in WebCore/platform/MainThreadTask.h. It certainly uses the WTF one behind the scenes, but doesn't force you to pass a fake void* argument.

I see!  I did Source/-wide search for all users of callOnMainThread and noticed that nobody called the argument-free version, so I passed on it...  but I'll keep this in mind next time!

[Brady Eidson]

It might be worth a global replace, as the fake void* argument idiom is pretty entrenched.