Bug 199320 – Remove IDBValue::m_sessionID

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Remove IDBValue::m_sessionID

[youenn fablet]

Created attachment 373125 [details]
Patch

[EWS Watchlist]

Comment on attachment 373125 [details]
Patch

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

New failing tests:
jquery/offset.html

[EWS Watchlist]

Created attachment 373139 [details]
Archive of layout-test-results from ews116 for mac-highsierra

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

[youenn fablet]

Created attachment 375578 [details]
Patch

[Alex Christensen]

Comment on attachment 375578 [details]
Patch

Please don't. This is work in progress in an attempt to make the BlobRegistry owned by the NetworkSession instead of the NetworkProcess, which will make blobs and service workers not require a strange workaround that is probably a privacy issue.

[youenn fablet]

(In reply to Alex Christensen from comment #5)
> Comment on attachment 375578 [details]
> Patch
> 
> Please don't. This is work in progress in an attempt to make the
> BlobRegistry owned by the NetworkSession instead of the NetworkProcess,
> which will make blobs and service workers not require a strange workaround
> that is probably a privacy issue.

Can we try finding another way of getting the session IDs than from the IDBValue?
What is exactly the case where it is currently difficult to get a session ID?

In NetworkProcess, it seems that we should be able to get the session ID from the IDBServer that is manipulating the IDBValue, or from the IPC message that carries the IDBValue.

In several cases, IDBValue has no session ID (i.e. an empty sessionID) right now (maybe because the implementation is incomplete?) and these are dangerous IDs that I would like to make IPC deserialization fail.

[youenn fablet]

(In reply to Alex Christensen from comment #5)
> Comment on attachment 375578 [details]
> Patch
> 
> Please don't. This is work in progress in an attempt to make the
> BlobRegistry owned by the NetworkSession instead of the NetworkProcess,
> which will make blobs and service workers not require a strange workaround
> that is probably a privacy issue.

Serialization/deserialization routines are unaffected by sessionIDs since they go from bytes to structures and blob bytes or blob URLs only.
Since we do not want to store session IDs on disk, I do not see why session IDs should be passed around in the routines.

Agreed that wherever blobRegistry is used, we need to pass a sessionID so that we end up with a good partitioned blob registry.
This is the case in SerializedScriptValue::writeBlobsToDiskForIndexedDB/writeBlobsToDiskForIndexedDBSynchronously where we can pass the sessionID as a parameter.

[Alex Christensen]

We spoke in person and agreed to make them an Optional<SessionID>

[youenn fablet]

Created attachment 375689 [details]
Passing sessionID when writing blobs

[youenn fablet]

Created attachment 375731 [details]
Patch

[youenn fablet]

(In reply to Alex Christensen from comment #8)
> We spoke in person and agreed to make them an Optional<SessionID>

Done.

I went further and made a patch in bug 200567 to make Blob handling between WebProcess/NetworkProcess sessionID-aware. This for instance allows removing the service-worker specific hack.

This requires a minor modification to SerializedScriptValue, given bug 200567 makes Blob storing a session ID so that we can register/unregister blob URLs more easily. The sessionID is retrieved from the JSDOMGlobalObject given to the deserialiser.

It seems to me we should proceed with removing IDBValue::m_sessionID.

[WebKit Commit Bot]

Comment on attachment 375731 [details]
Patch

Clearing flags on attachment: 375731

Committed r248527: <https://trac.webkit.org/changeset/248527>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54204438>