Summary: | Loads are never canceled in the NetworkProcess | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brady Eidson <beidson> | ||||
Component: | WebKit2 | Assignee: | Brady Eidson <beidson> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | andersca, ap, levin+threading, sam, webkit.review.bot | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Brady Eidson
2013-03-11 22:20:44 PDT
Created attachment 192641 [details]
Patch v1
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. (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! (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 > 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.
(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! It might be worth a global replace, as the fake void* argument idiom is pretty entrenched. |