Since r240363 "Deprecate API to limit the maximum number of WebProcesses" several inherently-unsafe WebKitWebContext APIs no longer work: webkit_web_context_set_web_process_count_limit webkit_web_context_get_web_process_count_limit The process count limit APIs are no longer safe because we assume websites that abuse Spectre are able to read arbitrary memory from the web process, which could allow one security origin to compromise sensitive data from another after the process count limit is reached. We should deprecate those without replacement. This is easy to do because it shouldn't affect the correctness of application code unless the application is doing something weird. typedef enum { WEBKIT_PROCESS_MODEL_SHARED_SECONDARY_PROCESS, WEBKIT_PROCESS_MODEL_MULTIPLE_SECONDARY_PROCESSES, } WebKitProcessModel; webkit_web_context_set_process_model webkit_web_context_get_process_model These ones are trickier. r240363 allows the special case of one secondary process, which is intended for debugging, but could be useful by many native apps that render trusted content. So in theory, we could keep these functions working as-is and without deprecation, though we should change the default process model to WEBKIT_PROCESS_MODEL_MULTIPLE_SECONDARY_PROCESSES and make sure our documentation explains the speculative execution attacks that will be enabled by WEBKIT_PROCESS_MODEL_SHARED_SECONDARY_PROCESS. But the problem is, we have to set APIProcessPoolConfiguration::m_usesSingleWebProcess before creating the WebProcessPool, which is too soon to keep webkit_web_context_set_process_model() working. Users call webkit_web_context_set_process_model() after the WebKitWebContext has been constructed (at which time the WebProcessPool has been constructed) but before constructing any WebKitWebViews with it (at which time the first web process is launched and it becomes too late). So if we want to keep this working, we'd need to work with Brady to make some changes to allow setting the process limit to 1 in some other manner. Otherwise, deprecating seems reasonable too, and safer. All of the above assumes we switch on PSON, which probably makes sense *after* branching 2.24. (Without PSON, multiple secondary process mode is not safe either.) So we could, for example, make these changes on trunk for now, but revert them and r240363 on the 2.24 branch. Probably some API tests are broken, and will need to be updated. In Epiphany, these are hidden settings, which I'll just remove because it's not safe for a web browser. We've kept them around so far because some people need it to limit memory usage, but Adrian is about to land some massive improvements there (as much as 5x web process memory reduction with content extensions) so they won't be needed anymore.
We are supposed to branch in two weeks, we could also branch right now after r240363, but still allow to land the pending features API in the branch for a while. Then we can decide what to do in trunk. We can probably deprecate both APIs, and keep the single process mode as a debugging option using an env var (you love them). What's exactly PSON?
(In reply to Carlos Garcia Campos from comment #1) > We are supposed to branch in two weeks, we could also branch right now after > r240363, but still allow to land the pending features API in the branch for > a while. Then we can decide what to do in trunk. Either way seems fine. > We can probably deprecate > both APIs, and keep the single process mode as a debugging option using an > env var (you love them). If we add an env var, let's limit it to developer mode. > What's exactly PSON? Process swap on navigation. Status quo: load google.com then apple.com in the same web view, only one web process is used, apple.com can potentially steal sensitive data from google.com because it can read process memory via speculative execution attack PSON: load google.com then apple.com, new web process is launched for the same web view, danger is mitigated because the two security origins share different processes Probably there will be problems, so not a great thing to enable at the end of the release cycle.
Created attachment 360313 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
We might want to change the deprecated tags to Deprecated: 2.26, if we revert r240363 on the 2.24 branch.
Carlos, what do you want to do with this...?
(In reply to Michael Catanzaro from comment #5) > We might want to change the deprecated tags to Deprecated: 2.26, if we > revert r240363 on the 2.24 branch. We decided to do this.
Created attachment 361057 [details] Patch
Note: my patch does not deprecate WebKitProcessModel simply because that introduces a ton of deprecation warnings in WebKit's own build process. We might want to do that in a follow-up.
Comment on attachment 361057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361057&action=review Looks fine, just a small comment below with a suggestion. > Source/WebKit/UIProcess/API/gtk/WebKitWebContext.h:77 > * I think that the deprecation could be added conditionally to avoid the warnings while building WebKit, but still having them show for applications built using 2.26.x -- something like the following could work: #if !defined(WEBKIT2_COMPILATION) WEBKIT_DEPRECATED #endif typedef enum { ... } WebKitProcessModel;
Good idea.
Comment on attachment 361057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361057&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:86 > - * You can define the #WebKitCacheModel and #WebKitProcessModel with > - * webkit_web_context_set_cache_model() and > - * webkit_web_context_set_process_model(), depending on the needs of > + * You can define the #WebKitCacheModel with > + * webkit_web_context_set_cache_model(), depending on the needs of hmm, are we sure there won't be process models in the future? will PSON be default? maybe it's safer to only deprecate the specific single process model.
Comment on attachment 361057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361057&action=review >> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:86 >> + * webkit_web_context_set_cache_model(), depending on the needs of > > hmm, are we sure there won't be process models in the future? will PSON be default? maybe it's safer to only deprecate the specific single process model. Hm, I'm not sure, we might indeed add more process models in the future. But we do want the documentation to reflect that the process model is currently ignored and specifying it won't do anything. I'm open to suggestions as to how to handle the documentation.
Note my current patch does not deprecate WebKitProcessModel (although it is no longer used for any purpose).
I forgot to update the documentation of webkit_web_view_new_with_related_view(). Need to do that too, though I'm not really sure how since it's a security hole. Need to ask Apple about it.
(In reply to Michael Catanzaro from comment #14) > Note my current patch does not deprecate WebKitProcessModel (although it is > no longer used for any purpose). I would only deprecate the specific process model, not the functions get/set and document that when using the deprecated one, the value will be ignored and the default one will be used.
(In reply to Carlos Garcia Campos from comment #16) > I would only deprecate the specific process model, not the functions get/set > and document that when using the deprecated one, the value will be ignored > and the default one will be used. Do you want to try submitting a patch? It just seems too weird to me. My suggestion is to deprecate the functions now, and just un-deprecate them in the future in the unlikely event we ever introduce a new process model. Seems more likely that we'll never do that. And if we do, applications will have to be changed anyway if they want to use the new process model. In the meantime, it doesn't make sense for applications to continue using these functions.
(Don't forget about this one.)
BTW this is why TestMultiprocess is broken currently.
Ping reviewers.
Comment on attachment 361057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361057&action=review >>> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:86 >>> - * You can define the #WebKitCacheModel and #WebKitProcessModel with >>> - * webkit_web_context_set_cache_model() and >>> - * webkit_web_context_set_process_model(), depending on the needs of >>> + * You can define the #WebKitCacheModel with >>> + * webkit_web_context_set_cache_model(), depending on the needs of >> >> hmm, are we sure there won't be process models in the future? will PSON be default? maybe it's safer to only deprecate the specific single process model. > > Hm, I'm not sure, we might indeed add more process models in the future. But we do want the documentation to reflect that the process model is currently ignored and specifying it won't do anything. I'm open to suggestions as to how to handle the documentation. hmm, are we sure there won't be process models in the future? will PSON be default? maybe it's safer to only deprecate the specific single process model. > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1533 > - * This method **must be called before any web process has been created**, > - * as early as possible in your application. Calling it later will make > - * your application crash. > + * This function used to set the maximum number of web processes that > + * could be created at the same time for the @context, but it is no > + * longer safe to set such limits as process swapping is required to > + * mitigate speculative execution vulnerabilities. I prefer to keep the documentation unchanged and add a note saying it's deprecated and does nothing now. > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:-1560 > - > - if (context->priv->processCountLimit == limit) > - return; > - > - context->priv->processCountLimit = limit; And add a g_warning here too saying this does nothing. > Source/WebKit/UIProcess/API/gtk/WebKitWebContext.h:72 > + * @WEBKIT_PROCESS_MODEL_MULTIPLE_SECONDARY_PROCESSES: Use one or more > + * processes for each #WebKitWebView, while still allowing for some One or more web processes for each view? It's confusing, the fact that we can swap processes is an implementation detail, there's still always one web process per web view, in contrast to single process for all web views.
OK, I still think it's weird to have a non-deprecated function that doesn't do anything, but your solution is OK too so I'll try again to implement it. (In reply to Carlos Garcia Campos from comment #21) > > Source/WebKit/UIProcess/API/gtk/WebKitWebContext.h:72 > > + * @WEBKIT_PROCESS_MODEL_MULTIPLE_SECONDARY_PROCESSES: Use one or more > > + * processes for each #WebKitWebView, while still allowing for some > > One or more web processes for each view? It's confusing, the fact that we > can swap processes is an implementation detail, there's still always one web > process per web view, in contrast to single process for all web views. Hm, let's ask Chris (hey Chris!), but I think both your comment and my documentation are wrong. It's one process per security origin (right?). So a view will be backed by multiple web processes if it includes frames from different origins, right? The two separate origins are not permitted to run in the same process; otherwise, one origin can exfiltrate data from the other by abusing Spectre. (At least that's how Chrome is supposed to work. Chris, is that actually what you've implemented? Or is that harder?) And also multiple views can happily share the same process without issue as long as they share the same security origin, right? (Again, Chris, is that what you've implemented?)
Um, and then there's related views. Do related views from different origins still share the same process? I guess using a separate process for frames and related views might be quite difficult to implement. I assume that's the end goal, but not sure if we're there yet?
Hey Chris, could you answer the questions in comment #22 and comment #23 please?
Regarding comment #22: how would be a "security origin" distinguished? Evolution uses iframe-s, these (and the main body as well) can reference different URIs, like mail://, evo-http://, evo-https://, gtk-stock://, ... all of them being handled by URI handlers. I cannot imagine you run one iframe in one WebProcess and another iframe of the same WebView in another process. That will surely break all the expectations Evolution has on WebKitGTK+.
(In reply to Milan Crha from comment #25) > Regarding comment #22: how would be a "security origin" distinguished? > > Evolution uses iframe-s, these (and the main body as well) can reference > different URIs, like mail://, evo-http://, evo-https://, gtk-stock://, ... > all of them being handled by URI handlers. I cannot imagine you run one > iframe in one WebProcess and another iframe of the same WebView in another > process. That will surely break all the expectations Evolution has on > WebKitGTK+. Security origin is <protocol, host, port>, so yes those would all be different -security origins (often abbreviated as "origins"). This is, in fact, the eventual desired behavior afaik. I think it's what Chrome is doing also (or at least planning to do). It is nowadays a security vulnerability to have separate origins running in the same process. However, iframes can script each other, so it's not trivial to do and might not be implemented in WebKit yet (I'm not sure).
Created attachment 377875 [details] Patch
Created attachment 377876 [details] Patch
Committed r249419: <https://trac.webkit.org/changeset/249419>