Bug 161609 – CachedResource should efficiently construct its ResourceRequest

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

Currently it is doing a copy from a request owned by a CachedResourceRequest which is destroyed.

[youenn fablet]

Created attachment 287967 [details]
Patch

[youenn fablet]

Created attachment 287972 [details]
Fixing LinkLoader

[youenn fablet]

Created attachment 287974 [details]
Fixing EFL compilation issue

[youenn fablet]

Created attachment 287978 [details]
Fixing EFL compilation issue

[Build Bot]

Comment on attachment 287978 [details]
Fixing EFL compilation issue

Attachment 287978 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2013557

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-basic-worker.html

[Build Bot]

Created attachment 287980 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[youenn fablet]

Comment on attachment 287978 [details]
Fixing EFL compilation issue

PTest failure is unrelated to this patch.
I'll fix the test tomorrow

[Darin Adler]

Comment on attachment 287978 [details]
Fixing EFL compilation issue

Looks like this caused a test failure:

  imported/w3c/web-platform-tests/fetch/api/cors/cors-basic-worker.html [ Failure ]

[youenn fablet]

(In reply to comment #8)
> Comment on attachment 287978 [details]
> Fixing EFL compilation issue
> 
> Looks like this caused a test failure:
> 
>   imported/w3c/web-platform-tests/fetch/api/cors/cors-basic-worker.html [
> Failure ]

This test had an issue and will be fixed in  landing patch for bug 161389

[youenn fablet]

(In reply to comment #9)
> (In reply to comment #8)
> > Comment on attachment 287978 [details]
> > Fixing EFL compilation issue
> > 
> > Looks like this caused a test failure:
> > 
> >   imported/w3c/web-platform-tests/fetch/api/cors/cors-basic-worker.html [ Failure ]

Test should now be fine.

[Alex Christensen]

This looks related:

Regressions: Unexpected text-only failures (1)
  imported/w3c/web-platform-tests/fetch/api/cors/cors-basic-worker.html [ Failure ]

[youenn fablet]

(In reply to comment #11)
> This looks related:
> 
> Regressions: Unexpected text-only failures (1)
>   imported/w3c/web-platform-tests/fetch/api/cors/cors-basic-worker.html [
> Failure ]

This test file was running promise tests.
But the promise was not returned to tstharness so the test file was done before all promises were resolved. This was fixed in bug 161389.

This was noticeable here as we are starting to do some console logging stuff.
Sometimes the logs happened before the test was done sometimes after, hence the flakiness.

I can reupload this patch to resubmit it to EWS bots if that helps.

[youenn fablet]

Created attachment 288559 [details]
Rebasing

[Alex Christensen]

Comment on attachment 288559 [details]
Rebasing

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

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:689
> +    // FIXME: We probably moved request and should not reuse it here for safety.
> +    // We should refactor auickly this code.
>      if (!request.forPreload() || policy != Use)
>          resource->setLoadPriority(request.priority());

Yes, we should definitely store this bool before moving it.  We shouldn't land code that uses a variable we have WTFMoved from.

[youenn fablet]

Comment on attachment 288559 [details]
Rebasing

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

>> Source/WebCore/loader/cache/CachedResourceLoader.cpp:689
>>          resource->setLoadPriority(request.priority());
> 
> Yes, we should definitely store this bool before moving it.  We shouldn't land code that uses a variable we have WTFMoved from.

I saw that already in WebKit code base, maybe this should be in WebKit coding style.
I can do that change in that patch, although this will make a tad more difficult to review all the changes.

[youenn fablet]

Created attachment 288599 [details]
Fixed WTFMove related FIXMEs

[youenn fablet]

Created attachment 289049 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 289049 [details]
Patch for landing

Clearing flags on attachment: 289049

Committed r206016: <http://trac.webkit.org/changeset/206016>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.