Bug 54755 – REGRESSION (WebKit2): HTTP requests time out after 60 seconds

• Component: Page Loading
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

Safari changes NSURLRequest default timeout from 60 seconds to 1 year - but now that Safari is a different process, that has no effect on WebKit page loading.

<rdar://problem/9006592>

[Alexey Proskuryakov]

Created attachment 83201 [details]
proposed patch

[WebKit Review Bot]

Attachment 83201 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/Shared/WebURLRequest.h:64:  The parameter name "timeoutInterval" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Roben (:aroben)]

Comment on attachment 83201 [details]
proposed patch

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

I'm not sure I understand why we need this API on both WKURLRequest and WKContext.

> Source/WebKit2/Shared/WebURLRequest.cpp:29
> +    WebCore::ResourceRequest::setDefaultTimeoutInterval(timeoutInterval);

No need for the WebCore::

> Source/WebKit2/Shared/WebURLRequest.cpp:32
> +
> +

Extra blank line here.

> Source/WebKit2/WebProcess/WebProcess.cpp:233
> +    ResourceRequestBase::setDefaultTimeoutInterval(timeoutInterval);

Why "ResourceRequestBase" instead of "ResourceRequest", like WebURLRequest.cpp uses?

[Alexey Proskuryakov]

> I'm not sure I understand why we need this API on both WKURLRequest and WKContext.

From implementation point of view, one affects requests in the UI process, while another affects Web contexts. It seems unlikely indeed that one would ever want these different, but I'm not sure how we could do it with one API.

Presumably it would be the WKURLRequest one that would stay - how can I enumerate all contexts to notify them of a change?

[Adam Roben (:aroben)]

(In reply to comment #4)
> > I'm not sure I understand why we need this API on both WKURLRequest and WKContext.
> 
> From implementation point of view, one affects requests in the UI process, while another affects Web contexts. It seems unlikely indeed that one would ever want these different, but I'm not sure how we could do it with one API.
> 
> Presumably it would be the WKURLRequest one that would stay - how can I enumerate all contexts to notify them of a change?

I don't think there's an existing way to enumerate all contexts, but it would probably be pretty easy to add one.

[Alexey Proskuryakov]

Created attachment 83216 [details]
proposed patch

Hide it all under a WKURLRequest API.

[Adam Roben (:aroben)]

Comment on attachment 83216 [details]
proposed patch

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

> Source/WebKit2/UIProcess/WebProcessManager.h:44
> +    void getAllWebProcessContexts(Vector<WebContext*>&);

Is there any other kind of context? Maybe this should just be called getAllContexts?

[Alexey Proskuryakov]

A context may not have a process at the moment, in which case it doesn't need to be notified.

Also, we (theoretically?) have a single-process model where the "web process" runs on a separate thread.

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/79253>.