Bug 146710 - Move PingLoaders to the NetworkingProcess
Summary: Move PingLoaders to the NetworkingProcess
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
: 138688 (view as bug list)
Depends on:
Blocks: 146754
  Show dependency treegraph
 
Reported: 2015-07-07 22:34 PDT by Brady Eidson
Modified: 2015-07-09 08:20 PDT (History)
5 users (show)

See Also:


Attachments
Patch v1 (26.40 KB, patch)
2015-07-08 12:29 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch v2 (26.43 KB, patch)
2015-07-08 13:59 PDT, Brady Eidson
thorton: review+
Details | Formatted Diff | Diff
Patch for landing. (26.44 KB, patch)
2015-07-08 14:47 PDT, Brady Eidson
dbates: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (29.60 KB, patch)
2015-07-08 15:00 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch for landing. (26.45 KB, patch)
2015-07-08 15:04 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2015-07-07 22:34:53 PDT
Move PingLoaders to the NetworkingProcess

<rdar://problem/18860263>
Comment 1 Brady Eidson 2015-07-08 12:29:00 PDT
Created attachment 256395 [details]
Patch v1
Comment 2 Brady Eidson 2015-07-08 13:59:14 PDT
Created attachment 256403 [details]
Patch v2
Comment 3 Tim Horton 2015-07-08 14:08:33 PDT
Comment on attachment 256403 [details]
Patch v2

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

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:262
> +    WebFrame* webFrame = webFrameLoaderClient ? webFrameLoaderClient->webFrame() : 0;

nullptr!
Comment 4 Brady Eidson 2015-07-08 14:47:09 PDT
Created attachment 256407 [details]
Patch for landing.
Comment 5 Daniel Bates 2015-07-08 14:52:46 PDT
Comment on attachment 256407 [details]
Patch for landing.

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

> Source/WebCore/platform/network/PingHandle.h:65
> +    virtual bool usesAsyncCallbacks() { return m_usesAsyncCallbacks == UsesAsyncCallbacks::Yes; }

This needs to be marked override or it will cause a compiler error since we compile with -Winconsistent-missing-override.
Comment 6 Daniel Bates 2015-07-08 15:00:22 PDT
Created attachment 256408 [details]
Patch for landing

Added keyword override to the declaration of PingHandle::usesAsyncCallbacks.
Comment 7 Brady Eidson 2015-07-08 15:04:22 PDT
Created attachment 256409 [details]
Patch for landing.
Comment 8 Alexey Proskuryakov 2015-07-08 15:07:19 PDT
Comment on attachment 256408 [details]
Patch for landing

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

Finally! :)

> Source/WebCore/ChangeLog:9
> +        There's no current solution for communicating back to a page that a ping load has reached its target.

We could have the ping target change some global state (usually it's a file on disk), and then another document could poll the state.
Comment 9 Daniel Bates 2015-07-08 15:10:55 PDT
<rdar://problem/18860263>
Comment 10 Brady Eidson 2015-07-08 15:38:21 PDT
(In reply to comment #8)
> Comment on attachment 256408 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256408&action=review
> 
> Finally! :)
> 
> > Source/WebCore/ChangeLog:9
> > +        There's no current solution for communicating back to a page that a ping load has reached its target.
> 
> We could have the ping target change some global state (usually it's a file
> on disk), and then another document could poll the state.

That was my first idea, but then I realized that:
- Tests that rely on files on disk on the httpd side are often fragile
- Tests that rely on polling are... gross

I actually played around with running two php scripts and IPC'ing between them, and I think that will actually work, but there's a steep learning curve incompatible with getting this frick'in bug fix in!

I will revisit it soon - it's on my white board :)
Comment 11 Brady Eidson 2015-07-08 15:39:21 PDT
(In reply to comment #9)
> <rdar://problem/18860263>

That was in comment 1 ;)
Comment 12 Brady Eidson 2015-07-08 15:47:57 PDT
*** Bug 138688 has been marked as a duplicate of this bug. ***
Comment 13 WebKit Commit Bot 2015-07-08 15:54:47 PDT
Comment on attachment 256409 [details]
Patch for landing.

Clearing flags on attachment: 256409

Committed r186530: <http://trac.webkit.org/changeset/186530>
Comment 14 Alexey Proskuryakov 2015-07-09 08:05:20 PDT
> - Tests that rely on files on disk on the httpd side are often fragile

They actually aren't any more, as long as written carefully. We now make sure to have unique names (usually by using a sanitized test path name as temporary file name), and that works pretty reliably, even in most stress testing scenarios.

Some CGI scripts get the name implicitly (from Referer header field), others get it explicitly.
Comment 15 Brady Eidson 2015-07-09 08:20:54 PDT
(In reply to comment #14)
> > - Tests that rely on files on disk on the httpd side are often fragile
> 
> They actually aren't any more, as long as written carefully. We now make
> sure to have unique names (usually by using a sanitized test path name as
> temporary file name), and that works pretty reliably, even in most stress
> testing scenarios.
> 
> Some CGI scripts get the name implicitly (from Referer header field), others
> get it explicitly.

Good to know.

I could be able to use that plus a script that polls for the file before responding. That way the test itself won't have to poll the server - it can just wait for the response.

But as long as I'm polling for a file in php to do adhoc IPC between scripts, I could just use one of the "real" PHP IPC mechanisms.

Will revisit soon.