Bug 142612 – XMLHttpRequests should not prevent a page from entering PageCache

• Component: XML
• Status: RESOLVED FIXED

[Chris Dumez]

XMLHttpRequests should not prevent a page from entering PageCache.

Radar: <rdar://problem/19923085>

[Chris Dumez]

Created attachment 248495 [details]
WIP Patch

[Chris Dumez]

Created attachment 248541 [details]
Patch

[Chris Dumez]

Created attachment 248544 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 248544 [details]
Patch

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

> Source/WebCore/loader/DocumentLoader.cpp:111
> +        // Only image and XHR loads do prevent the page from entering the PageCache.
> +        switch (cachedResource->type()) {
> +        case CachedResource::RawResource:

Is this an accurate check for XHR? I think that there are more raw resources, such as icons, media, and maybe anything loaded by workers.

> Source/WebCore/xml/XMLHttpRequest.cpp:1298
> +        m_resumeTimer.startOneShot(0);

What if we are suspended again while the timer is pending?

[Chris Dumez]

Created attachment 248557 [details]
Patch

[Chris Dumez]

Comment on attachment 248544 [details]
Patch

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

>> Source/WebCore/loader/DocumentLoader.cpp:111
>> +        case CachedResource::RawResource:
> 
> Is this an accurate check for XHR? I think that there are more raw resources, such as icons, media, and maybe anything loaded by workers.

You are right, this is not strictly correct. I made it correct by making sure all the clients of the resource's clients are XHR. So there is now an isXHR() API on CachedResourceClient. This is a bit annoying but I did not find a better way to do it.

>> Source/WebCore/xml/XMLHttpRequest.cpp:1298
>> +        m_resumeTimer.startOneShot(0);
> 
> What if we are suspended again while the timer is pending?

Right, good catch. It is now handled in suspend().

[Alexey Proskuryakov]

Comment on attachment 248557 [details]
Patch

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

> Source/WebCore/loader/DocumentThreadableLoader.h:99
> +        bool isXHR() const override final;

I don't think that we use this abbreviation in code often, I'd prefer "isXMLHttpRequest".

> Source/WebCore/loader/cache/CachedResource.h:161
> +    bool isLoadedViaXHROnly() const;

Maybe "allClientsAreXMLHttpRequests"?

> Source/WebCore/xml/XMLHttpRequest.cpp:1255
> +    // Dot not attempt to suspend loading XMLHttpRequests in workers threads for now.
> +    if (!is<Document>(*scriptExecutionContext()))
> +        return !m_loader;

Not sure if I prefer a runtime check or an assertion that this function isn't called in workers (it cannot be called there now).

> Source/WebCore/xml/XMLHttpRequest.cpp:1309
> +    dispatchErrorEvents(eventNames().errorEvent);
> +    m_dispatchErrorOnResuming = false;

Is this the right order? Error event handlers can do many things, and it's not clear to me that anything even guarantees that the object is still alive after returning from one. I think that an event handler can probably also start a new load using this XHR object, and synchronously navigate away from the page (with a javascript: URL navigation), in which case we'd incorrectly overwrite the value of m_dispatchErrorOnResuming.

> Source/WebCore/xml/XMLHttpRequest.cpp:1314
> +    NoEventDispatchAssertion assertNoEventDispatch;

I presume that you ran tests in a debug build locally.

[Chris Dumez]

Created attachment 248588 [details]
Patch

[Chris Dumez]

Comment on attachment 248557 [details]
Patch

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

>> Source/WebCore/xml/XMLHttpRequest.cpp:1314
>> +    NoEventDispatchAssertion assertNoEventDispatch;
> 
> I presume that you ran tests in a debug build locally.

I did and it does not hit assertions, surprisingly.

[WebKit Commit Bot]

Comment on attachment 248588 [details]
Patch

Clearing flags on attachment: 248588

Committed r181480: <http://trac.webkit.org/changeset/181480>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.