Bug 199320 - Remove IDBValue::m_sessionID
Summary: Remove IDBValue::m_sessionID
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-28 09:42 PDT by youenn fablet
Modified: 2019-08-12 04:40 PDT (History)
8 users (show)

See Also:


Attachments
Patch (35.80 KB, patch)
2019-06-28 10:19 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-highsierra (3.05 MB, application/zip)
2019-06-28 12:17 PDT, Build Bot
no flags Details
Patch (30.34 KB, patch)
2019-08-05 16:30 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Passing sessionID when writing blobs (33.93 KB, patch)
2019-08-06 22:55 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (30.24 KB, patch)
2019-08-07 12:20 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2019-06-28 09:42:16 PDT
Remove IDBValue::m_sessionID
Comment 1 youenn fablet 2019-06-28 10:19:05 PDT
Created attachment 373125 [details]
Patch
Comment 2 Build Bot 2019-06-28 12:17:43 PDT
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
Comment 3 Build Bot 2019-06-28 12:17:45 PDT
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
Comment 4 youenn fablet 2019-08-05 16:30:30 PDT
Created attachment 375578 [details]
Patch
Comment 5 Alex Christensen 2019-08-05 16:38:01 PDT
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.
Comment 6 youenn fablet 2019-08-05 17:23:08 PDT
(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.
Comment 7 youenn fablet 2019-08-06 15:49:34 PDT
(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.
Comment 8 Alex Christensen 2019-08-06 15:59:46 PDT
We spoke in person and agreed to make them an Optional<SessionID>
Comment 9 youenn fablet 2019-08-06 22:55:47 PDT
Created attachment 375689 [details]
Passing sessionID when writing blobs
Comment 10 youenn fablet 2019-08-07 12:20:30 PDT
Created attachment 375731 [details]
Patch
Comment 11 youenn fablet 2019-08-09 04:08:52 PDT
(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.
Comment 12 WebKit Commit Bot 2019-08-12 04:39:45 PDT
Comment on attachment 375731 [details]
Patch

Clearing flags on attachment: 375731

Committed r248527: <https://trac.webkit.org/changeset/248527>
Comment 13 WebKit Commit Bot 2019-08-12 04:39:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2019-08-12 04:40:32 PDT
<rdar://problem/54204438>