ServiceWorkerClients.openWindow promise should reject if no WebPageProxy is created
Created attachment 456020 [details] Patch
Created attachment 456021 [details] Patch
Comment on attachment 456021 [details] Patch r=me
This patch is incorrect. We had multiple back and forths in the openWindow patch. There's no bug here.
Comment on attachment 456021 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456021&action=review In general, I strongly, strongly dislike patches that replace callback signatures with either "auto" or a using/typedef. It make this review much harder than it should've been, and will force future maintainers of the code into a similarly hard "just read the code" problem. I think it makes the code worse. Please consider not making those changes. > Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:191 > + auto innerCallback = [callback = WTFMove(callback), server = WeakPtr { *server }, origin = worker->origin()](std::optional<WebCore::PageIdentifier>&& pageIdentifier) mutable { > + if (!pageIdentifier) { > + callback(makeUnexpected(ExceptionData { TypeError, "openWindow failed opening a tab"_s })); > + return; > + } All the other changes in this patch are fine. But this one is wrong (and why I r-'ed the patch based on description alone) It is *OKAY AND EXPECTED* for the UI process client to return a nil WKWebView, which translates to a missing PageIdentiier, which *should* translate to a null WindowClient in the promise. If an API client purposefully denies a popup, the situation is not "an exception occurred", but rather "your attempt to openWindow resulted in a null WindowClient" We should not be directly and explicitly communicating API client policies to web pages like this.
> We should not be directly and explicitly communicating API client policies > to web pages like this. Are you saying that our goal is to hide what happened from the webpage? If so, I don't understand that goal, and also I don't think the proposed alternative to resolve with null achieves the goal.
(In reply to Geoffrey Garen from comment #6) > > We should not be directly and explicitly communicating API client policies > > to web pages like this. > > Are you saying that our goal is to hide what happened from the webpage? If > so, I don't understand that goal, and also I don't think the proposed > alternative to resolve with null achieves the goal. Resolving with null is the specified way to tell the service worker "your request to open a window went fine, but it did not result in a WindowClient you can access" The spec also makes it clear that rejecting these promises is for errors/exceptions. My argument is that the client refusing to open the window is expected, not an error/exception. It's not something actionable from the web content's standpoint. Nothing they can explore or fix. I'm not sure what the use case is for differentiating. And without a use case, I don't see why we'd do it.
(In reply to Brady Eidson from comment #7) > (In reply to Geoffrey Garen from comment #6) > > > We should not be directly and explicitly communicating API client policies > > > to web pages like this. > > > > Are you saying that our goal is to hide what happened from the webpage? If > > so, I don't understand that goal, and also I don't think the proposed > > alternative to resolve with null achieves the goal. > > Resolving with null is the specified way to tell the service worker "your > request to open a window went fine, but it did not result in a WindowClient > you can access" > > The spec also makes it clear that rejecting these promises is for > errors/exceptions. > > My argument is that the client refusing to open the window is expected, not > an error/exception. > > It's not something actionable from the web content's standpoint. Nothing > they can explore or fix. > > I'm not sure what the use case is for differentiating. And without a use > case, I don't see why we'd do it. I thought I added this example in my reply, but apparently forgot: According to the spec, only exceptions thrown from the Navigation algorithm should reject the promise. *Failed navigations* don't even throw an exception. Eg. a 404 or 500+ response don't even reject the promise. A failed navigation returns null, and I also don't see how the client refusing to open the window is specially different from a failed navigation.
> Resolving with null is the specified way to tell the service worker "your > request to open a window went fine, but it did not result in a WindowClient > you can access" It sounds like the concern is the best way to match the spec, then? FWIW, the spec just doesn't address this topic, since it assumes that the window will open. You could argue that 7.2 means the promise should neither resolve nor reject, since the queued task will never dequeue if it is queued to null. You could argue that 7.2.1 means throw an exception, since we will fail the "allowed to navigate" check, since null is not same-origin with us. Or you could argue that 7.7.1 means resolve with null, since null is not same-origin with us (though why not trigger the same logic earlier at 7.2.1?) Or you could argue that the general spirit of the spec means resolve with null, since the spec only requires an exception if the URL is invalid, or there's a same-origin violation, or there's no user gesture, and if we assume that null is not obviously not-same-origin with us then this condition is not obviously one of those conditions, and we should be parsimonious with exceptions. I see how this ambiguity could give rise to debate, but should it really rise to the level of an r-? I would tend to think that any of these behaviors might be reasonable, and the best next step would be to land a patch with a reasonable guess and file a github issue to specify this behavior.
FWIW, the main goal of this patch is to make sure that if delegate is not implemented (which will happen for lots of apps), we do as if openWindow is not implemented and reject the promise. I think this is desireable behavior. Whether we should reject or resolve with null in case of a blocked navigation is a different but interesting debate.
(In reply to Geoffrey Garen from comment #9) > > Resolving with null is the specified way to tell the service worker "your > > request to open a window went fine, but it did not result in a WindowClient > > you can access" > > It sounds like the concern is the best way to match the spec, then? > > FWIW, the spec just doesn't address this topic, since it assumes that the > window will open. > > You could argue that 7.2 means the promise should neither resolve nor > reject, since the queued task will never dequeue if it is queued to null. This could be argued (but I am not arguing it). > You could argue that 7.2.1 means throw an exception, since we will fail the > "allowed to navigate" check, since null is not same-origin with us. Let's set aside the issue that the "Navigate" steps are nonsensical in this context because a service worker instance is by definite not a "source browsing context"... This scenario does *not* fail "allowed to navigate". It doesn't fail #1 because B is a top level browsing context It doesn't fail #2 because A is not an ancestor of B It doesn't fail #3 because we don't have the sandbox flags in play It falls into #4 - true > > Or you could argue that 7.7.1 means resolve with null, since null is not > same-origin with us (though why not trigger the same logic earlier at 7.2.1?) I AM arguing 7.7.1 means resolve with null. See above on why 7.2.1 doesn't apply. > Or you could argue that the general spirit of the spec means resolve with > null, I am arguing the general spirit of the spec means resolve with null unless something exceptional happened or the calling script did something disallowed or malformed. > I see how this ambiguity could give rise to debate, but should it really > rise to the level of an r-? Yes, and here's why: > I would tend to think that any of these > behaviors might be reasonable, and the best next step would be to land a > patch with a reasonable guess and file a github issue to specify this > behavior. The current behavior in the tree was r+'ed and landed. The proposed patch is therefore the one that's given rise to debate. The default should not be "let's land the patch that is causing debate then sort it out later", it should be "let's leave things where they are and sort it out later." This patch does great things that should be landed - except for this one change, which I believe makes us go against both the letter and spirit of the spec. I'd happily r+ this patch without the "null WKWebView means rejecting with an error" change.
> > You could argue that 7.2 means the promise should neither resolve nor > > reject, since the queued task will never dequeue if it is queued to null. > > This could be argued (but I am not arguing it). OK -- but what is your response to this argument, given your strident belief that the spec mandates resolving with null?
(In reply to Geoffrey Garen from comment #12) > > > You could argue that 7.2 means the promise should neither resolve nor > > > reject, since the queued task will never dequeue if it is queued to null. > > > > This could be argued (but I am not arguing it). > > OK -- but what is your response to this argument, given your strident belief > that the spec mandates resolving with null? My response would be that never resolving the promise would be nonsensical, and is probably not the intention of the spec.
> The current behavior in the tree was r+'ed and landed. I r+ed it and agreed to land it in the interest of time, not because I agreed with that behavior. At the time of my r+, I provided feedback about these edge cases, in particular about what Chrome and Firefox are doing there. I had a look at Chrome code. Chrome is rejecting the promise in some cases that the spec does not define, like openWindow('file://'). I filed https://bugs.webkit.org/show_bug.cgi?id=238658 to keep track of that. Chrome has the infrastructure to reject the promise based on the openWindow result gathered from the main process: https://source.chromium.org/chromium/chromium/src/+/main:content/browser/service_worker/service_worker_client_utils.cc;l=429;drc=7fb345a0da63049b102e1c0bcdc8d7831110e324. A few things worth noting (from code inspection so this needs actual validation): - It seems the default implementation of the callback (no tab is created) will trigger rejection of the promise. This is inline with this patch. - If openWindow triggers creation of a tab that quickly got closed, promise is resolved with null. This is inline with what we are doing with and without the patch. > The proposed patch is therefore the one that's given rise to debate. Not really, the debate was already there at the initial patch time. > I'd happily r+ this patch without the "null WKWebView means rejecting with > an error" change. Let's do that for now and let's continue the discussion on what we want/what others are doing. I filed https://github.com/w3c/ServiceWorker/issues/1639 to follow-up on the spec side.
Created attachment 456349 [details] Patch
Created attachment 456351 [details] Patch
Uploaded a new patch that does not reject in case of no page identifier here, this should hopefully get consensus. I uploaded a follow-up patch at https://bugs.webkit.org/show_bug.cgi?id=238664, which: - rejects in case of no page identifier - always return a page identifier if the delegate to create one returns a page (Meaning that a blocked load would end up resolving with null). Let's continue the discussion there on the exact behavior we want for those edge cases.
> > OK -- but what is your response to this argument, given your strident belief > > that the spec mandates resolving with null? > > My response would be that never resolving the promise would be nonsensical, > and is probably not the intention of the spec. Why is a promise that does not resolve nonsensical? There are plenty of spec-compliant ways to make a promise that does not resolve. For example, "new Promise". Or simply closing the window ("Remove any tasks associated with document in any task source, without running those tasks...."). Why should we stretch a probably guess at the intention behind a spec to the point where it contradicts the spec's plain written meaning? Wouldn't that approach invalidate the written texts of all specs, in favor of our own preferences? At this point, my best understanding of your feedback is, "I have an opinion about the best behavior here". It's OK to have an opinion! But I don't think it's fair to marshal the current spec in favor of your opinion when the spec's plain written meaning ("Queue a task to run the following steps on newContext’s Window object....") contradicts. It's also not obvious to me that spec discussion will ultimately change the spec in favor of your opinion, given that at least one other spec participant (Youenn) has a different opinion. So an assertion about spec "intention" would seem to lack grounding, or to be a projection of your own intention (which is OK, since you are a spec participant, but not definitive). On the meta level, we should be thoughtful and parsimonious about the use of r-, and especially r- overriding an r+, when a patch includes tests, passes existing tests, and otherwise conforms to project guidelines. The purpose of patch review is primarily mentoring and learning rather than gatekeeping, and "I have a conflicting opinion" feels more toward the gatekeeping side of the spectrum, especially when the opinion is about a a substantially rare edge case (like, do we even know how to make this case happen in real life, outside an API test?).
<rdar://problem/91290460>
Comment on attachment 456351 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456351&action=review r=me > Source/WebCore/workers/service/server/SWServer.cpp:123 > + if (clientIterator->value->frameType == ServiceWorkerClientFrameType::TopLevel && clientIterator->value->pageIdentifier && *clientIterator->value->pageIdentifier == pageIdentifier) `&& clientIterator->value->pageIdentifier && *clientIterator->value->pageIdentifier == pageIdentifier` can be replaced with: `&& clientIterator->value->pageIdentifier == pageIdentifier` std::optional's operator== will do the right thing for you. > Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:202 > + m_connection.networkProcess().parentProcessConnection()->sendWithAsyncReply(Messages::NetworkProcessProxy::OpenWindowFromServiceWorker { m_connection.sessionID(), url.string(), worker->origin().clientOrigin }, WTFMove(innerCallback)); Seems we should just IPC the URL, not the URL as a String. Looks like we end up needing a URL in the UIProcess eventually anyway. > Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:2970 > +static const char* ServiceWorkerWindowClientOpenWindowMain = static constexpr auto > Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:3011 > +"});"; Needs a ""_s suffix (note that I am about to make the String(const char*) constructor explicit to force people to use ASCIILiteral. > Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:3029 > + [preferences _setEnabled:NO forInternalDebugFeature:feature]; Seems you can break; after finding the setting you care about. > Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:3038 > + { "/", { ServiceWorkerWindowClientOpenWindowMain } }, "/"_s > Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:3039 > + { "/sw.js", { {{ "Content-Type", "application/javascript" }}, ServiceWorkerWindowClientOpenWindowJS } } Missing 3 _s suffixes.
(In reply to Geoffrey Garen from comment #18) > > > OK -- but what is your response to this argument, given your strident belief > > > that the spec mandates resolving with null? > > > > My response would be that never resolving the promise would be nonsensical, > > and is probably not the intention of the spec. > > Why is a promise that does not resolve nonsensical? There are plenty of > spec-compliant ways to make a promise that does not resolve. For example, > "new Promise". Or simply closing the window ("Remove any tasks associated > with document in any task source, without running those tasks...."). Not nonsensical, then. But also probably not the intention of the spec. > Why should we stretch a probably guess at the intention behind a spec to the > point where it contradicts the spec's plain written meaning? Wouldn't that > approach invalidate the written texts of all specs, in favor of our own > preferences? I don't follow this point: Are the written tests that show my reading of the spec is wrong? > At this point, my best understanding of your feedback is, "I have an opinion > about the best behavior here". It's OK to have an opinion! > > But I don't think it's fair to marshal the current spec in favor of your > opinion when the spec's plain written meaning ("Queue a task to run the > following steps on newContext’s Window object....") contradicts. It sounds like you have a 3rd opinion of the specced behavior, which IMHO further supports not changing things! > It's also not obvious to me that spec discussion will ultimately change the > spec in favor of your opinion, given that at least one other spec > participant (Youenn) has a different opinion. So an assertion about spec > "intention" would seem to lack grounding, or to be a projection of your own > intention (which is OK, since you are a spec participant, but not > definitive). I don't see the point in committing to one behavior (like, literally committing it to the tree), then changing the behavior we commit to, when there's no agreement. > On the meta level, we should be thoughtful and parsimonious about the use of > r-, and especially r- overriding an r+, when a patch includes tests, passes > existing tests, and otherwise conforms to project guidelines. By this logic, any patch that includes tests, passes existing tests, and otherwise conforms to project guidelines is immune to a second review, even if a second reviewer has a concretely objective reason to r- (e.g. an unambiguous reading of the spec that the author and first reviewer missed) I don't think this logic is fleshed out enough to be a hard-and-fast rule. My r- here was not meant to be a judgement on anyones character, competency, code quality, review quality, etc etc. "Not meant to be taken personally" It was meant to be more of an emergency stop to a patch that I saw ready to land - and perhaps moments away from landing - that I spotted a problem with that the author and reviewer didn't spot. I've been the one in the position of having written a patch, gotten an r+, and then had an r- come along after because something was spotted that should stop it. I didn't take it personally or think it was counterproductive. I've also been the one in the position of having r+'ed a patch, then had an r- come shortly therefore having spotted something not original spotted. I didn't take it personally or think it was counterproductive. I don't think it would be healthy for the project to have a policy where if a second reviewer spots a problem with a patch that hasn't quite landed yet, they delay it a bit. > The purpose of patch review is primarily mentoring and learning rather than gatekeeping, > and "I have a conflicting opinion" feels more toward the gatekeeping side of > the spectrum, especially when the opinion is about a a substantially rare > edge case (like, do we even know how to make this case happen in real life, > outside an API test?). This patch and review (bizarrely to me!) has been uncommonly contentious. My intention was not gatekeeping the entire patch. As implicitly and explicitly expressed at various stages, my issue was one *one very small piece of the patch*, and I wasn't trying to "flex" or contend that my opinion was more valuable then the author's or reviewer's. My take was: "This patch does good things, but this one small part of it seems very wrong to me. Let's not land it with this one small part that seems very wrong to me until we can come to a consensus"
Thanks for the review. > > Source/WebCore/workers/service/server/SWServer.cpp:123 > > + if (clientIterator->value->frameType == ServiceWorkerClientFrameType::TopLevel && clientIterator->value->pageIdentifier && *clientIterator->value->pageIdentifier == pageIdentifier) > > `&& clientIterator->value->pageIdentifier && > *clientIterator->value->pageIdentifier == pageIdentifier` > can be replaced with: > `&& clientIterator->value->pageIdentifier == pageIdentifier` > > std::optional's operator== will do the right thing for you. Done > > Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:202 > > + m_connection.networkProcess().parentProcessConnection()->sendWithAsyncReply(Messages::NetworkProcessProxy::OpenWindowFromServiceWorker { m_connection.sessionID(), url.string(), worker->origin().clientOrigin }, WTFMove(innerCallback)); > > Seems we should just IPC the URL, not the URL as a String. Looks like we end > up needing a URL in the UIProcess eventually anyway. Agreed, but this is preexisting, let's do that as a follow-up. > > Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:2970 > > +static const char* ServiceWorkerWindowClientOpenWindowMain = > > static constexpr auto > > > Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:3011 > > +"});"; > > Needs a ""_s suffix (note that I am about to make the String(const char*) > constructor explicit to force people to use ASCIILiteral. > > > Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:3029 > > + [preferences _setEnabled:NO forInternalDebugFeature:feature]; > > Seems you can break; after finding the setting you care about. > > > Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:3038 > > + { "/", { ServiceWorkerWindowClientOpenWindowMain } }, > > "/"_s > > > Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:3039 > > + { "/sw.js", { {{ "Content-Type", "application/javascript" }}, ServiceWorkerWindowClientOpenWindowJS } } > > Missing 3 _s suffixes. OK
Created attachment 456787 [details] Patch for landing
Created attachment 456791 [details] Patch for landing
Committed r292456 (249307@main): <https://commits.webkit.org/249307@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 456791 [details].