Bug 200616 – Remove DeferredPromise::sessionID()

• Component: Page Loading
• Status: RESOLVED FIXED

[youenn fablet]

Remove DeferredPromise::sessionID()

[youenn fablet]

Created attachment 376017 [details]
Patch

[EWS Watchlist]

Comment on attachment 376017 [details]
Patch

Attachment 376017 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12894156

New failing tests:
imported/w3c/web-platform-tests/fetch/api/request/request-consume-empty.html
http/wpt/fetch/request-stream-empty.html
http/wpt/fetch/fetch-as-blob-worker.html
imported/w3c/web-platform-tests/fetch/api/request/request-init-003.sub.html
imported/w3c/web-platform-tests/fetch/api/response/response-consume-empty.html
media/track/track-in-band-metadata-display-order.html
imported/w3c/web-platform-tests/fetch/api/request/request-consume.html
imported/w3c/web-platform-tests/mimesniff/mime-types/parsing.any.html
imported/w3c/web-platform-tests/fetch/api/response/response-consume.html
http/wpt/fetch/fetch-as-blob.html
imported/w3c/web-platform-tests/mimesniff/mime-types/parsing.any.worker.html

[EWS Watchlist]

Created attachment 376038 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[youenn fablet]

Created attachment 376045 [details]
Patch

[Darin Adler]

Comment on attachment 376045 [details]
Patch

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

Not sure I understand 100%.

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:69
> +        promise->resolveCallbackValueWithNewlyCreated<IDLInterface<Blob>>([&](auto& context) {
> +            return blobFromData(context.sessionID(), data, length, contentType);
> +        });

Would be more obvious that it’s safe if we captured a local variable containing the session ID rather than a reference to the context object itself. A little less elegant, but easier to be sure there’s no object lifetime problem.

Unless session IDs can change over time?

[youenn fablet]

(In reply to Darin Adler from comment #5)
> Comment on attachment 376045 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376045&action=review
> 
> Not sure I understand 100%.

The lambda is only called once we are sure the promise can be actually resolved.
It can only be resolved if the context is valid.
This has two benefits:
- It only creates the blob if needed
- It passes the context, which can be used for creating the C++ object or for getting the session ID.

We could also try to stick the session ID into FetchBodyConsumer but this would require to pipe it from FetchRequest/FetchResponse up to FetchBodyConsumer, and we would anyway get it initially from the context.
It seems nicer to get it from the promise itself when actually useful.

> > Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:69
> > +        promise->resolveCallbackValueWithNewlyCreated<IDLInterface<Blob>>([&](auto& context) {
> > +            return blobFromData(context.sessionID(), data, length, contentType);
> > +        });
> 
> Would be more obvious that it’s safe if we captured a local variable
> containing the session ID rather than a reference to the context object
> itself. A little less elegant, but easier to be sure there’s no object
> lifetime problem.
> 
> Unless session IDs can change over time?

The session ID cannot change.
The lambda is called synchronously, since we need to call the JS built-in resolve function with the JS value.
I can change the lambda to capture [&data, &length, &contentType].

[youenn fablet]

Created attachment 376264 [details]
Patch

[youenn fablet]

> This has two benefits:
> - It only creates the blob if needed

For instance, we currently allocate a vector of char to create the blob that will be the value given to the promise. In some cases, we create this value for nothing.
With the current patch, the blob will not be created if not needed.

> > Unless session IDs can change over time?
> 
> The session ID cannot change.

The session ID can change in theory. 
I believe this happens in practice for testing only. The implementation is probably not working super well since we are storing the session ID in various places that we would need to update (workers, IDB/SW/Cache connections...) if changing the page/document session ID.

[WebKit Commit Bot]

Comment on attachment 376264 [details]
Patch

Clearing flags on attachment: 376264

Committed r248897: <https://trac.webkit.org/changeset/248897>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54504452>