Skip to content
Permalink
Browse files
ServiceWorkers API should reject promises when calling objects inside…
… detached frames

https://bugs.webkit.org/show_bug.cgi?id=180444

Reviewed by Youenn Fablet.

LayoutTests/imported/w3c:

Rebaseline test now that it is passing some checks.

* web-platform-tests/service-workers/service-worker/detached-context.https-expected.txt:
* web-platform-tests/service-workers/service-worker/register-closed-window.https-expected.txt:

Source/WebCore:

ServiceWorkers API should reject promises when calling objects inside detached frames.

No new tests, rebaselined existing test.

* bindings/js/JSDOMPromiseDeferred.h:
(WebCore::callPromiseFunction):
Use the caller's globalObject instead of the lexicalGlobalObject when constructing the
deferred promise. The bug became visible when working on this service worker bug since
rejecting the promise when the frame is detached did not actually work. The issue is
that since the promise was created with the detached frame's globalObject, then it was
suspended and would not run script.

* bindings/js/JSDOMWindowBase.cpp:
(WebCore::callerGlobalObject):
(WebCore::incumbentDOMWindow):
* bindings/js/JSDOMWindowBase.h:
Add convenience function to get the caller's globalObject. It was carved out of
incumbentDOMWindow().

* workers/service/ServiceWorker.cpp:
(WebCore::ServiceWorker::postMessage):
* workers/service/ServiceWorkerContainer.cpp:
(WebCore::ServiceWorkerContainer::addRegistration):
(WebCore::ServiceWorkerContainer::getRegistration):
(WebCore::ServiceWorkerContainer::getRegistrations):
* workers/service/ServiceWorkerRegistration.cpp:
(WebCore::ServiceWorkerRegistration::update):
(WebCore::ServiceWorkerRegistration::unregister):
Reject the promise when m_isStopped flag is set (i.e. ActiveDOMObject::stop()
has been called).

LayoutTests:

* TestExpectations:
Unskip test that no longer times out and starts passing a few checks.

* fast/dom/navigator-detached-no-crash-expected.txt:
Rebaseline test now that promise is rejected.

* http/tests/media/media-stream/disconnected-frame-permission-denied-expected.txt:
* http/tests/media/media-stream/disconnected-frame-permission-denied.html:
Update and rebaseline test now that the promise is rejected. I verified that this
behavior is consistent with Chrome.


Canonical link: https://commits.webkit.org/196405@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@225577 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
cdumez committed Dec 6, 2017
1 parent 48899e2 commit 12f5d12fdb5118b2d75b75e4d4ae84d4623b3e5f
Showing 24 changed files with 148 additions and 64 deletions.
@@ -1,3 +1,21 @@
2017-12-06 Chris Dumez <cdumez@apple.com>

ServiceWorkers API should reject promises when calling objects inside detached frames
https://bugs.webkit.org/show_bug.cgi?id=180444

Reviewed by Youenn Fablet.

* TestExpectations:
Unskip test that no longer times out and starts passing a few checks.

* fast/dom/navigator-detached-no-crash-expected.txt:
Rebaseline test now that promise is rejected.

* http/tests/media/media-stream/disconnected-frame-permission-denied-expected.txt:
* http/tests/media/media-stream/disconnected-frame-permission-denied.html:
Update and rebaseline test now that the promise is rejected. I verified that this
behavior is consistent with Chrome.

2017-12-06 Matt Lewis <jlewis3@apple.com>

Marked storage/indexeddb/modern/idbtransaction-objectstore-failures-private.html as flaky on macOS.
@@ -145,7 +145,6 @@ imported/w3c/web-platform-tests/secure-contexts/shared-worker-secure-first.https

# Skip service worker tests that are timing out.
imported/w3c/web-platform-tests/fetch/api/abort/general-serviceworker.https.html [ Skip ]
imported/w3c/web-platform-tests/service-workers/service-worker/detached-context.https.html [ Skip ]
imported/w3c/web-platform-tests/service-workers/service-worker/extendable-event-waituntil.https.html [ Skip ]
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-event-respond-with-partial-stream.https.html [ Skip ]
imported/w3c/web-platform-tests/service-workers/service-worker/http-to-https-redirect-and-register.https.html [ Skip ]
@@ -1,3 +1,4 @@
CONSOLE MESSAGE: Unhandled Promise Rejection: TypeError: Not enough arguments
This tests that the navigator object of a deleted frame is disconnected properly. Accessing fields or methods shouldn't crash the browser.
Check Navigator
navigator.appCodeName is OK
@@ -1,4 +1,3 @@
frame "<!--framePath //<!--frame0-->-->" - has 1 onunload handler(s)
Tests that when a request is made on a UserMedia object and its Frame is disconnected before a callback is made, the error callback is invoked with the correct error message.

On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
@@ -1,12 +1,11 @@
frame "<!--framePath //<!--frame0-->-->" - has 1 onunload handler(s)
Tests that when a getUserMedia request is made, permission is denied and its Frame is disconnected before a callback is made, the error callback is invoked with PERMISSION_DENIED.

On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".


PASS error.name is "NotAllowedError"

PASS No callbacks invoked
PASS Error callback invoked
PASS successfullyParsed is true

TEST COMPLETE
@@ -17,6 +17,7 @@
var options = {audio: true, video: true};
function onIframeLoaded() {
iframeNavigator = iframe.contentWindow.navigator;
iframe.contentWindow.onunload = onIframeUnloaded;
iframeNavigator.mediaDevices.getUserMedia(options)
.then(stream => {
testFailed('Success callback invoked unexpectedly');
@@ -38,11 +39,11 @@
finishJSTest();
})
.catch(err => {
testFailed('Error callback invoked unexpectedly');
testPassed('Error callback invoked');
finishJSTest();
});
setTimeout(function() {
testPassed('No callbacks invoked');
testFailed('No callbacks invoked');
finishJSTest();
}, 100);
}
@@ -13,7 +13,8 @@

function onIframeLoaded() {
iframeNavigator = iframe.contentWindow.navigator;
iframe.src = 'data:text/html,This frame should be visible when the test completes';
iframe.remove();
onIframeUnloaded();
}

function onIframeUnloaded() {
@@ -2,7 +2,7 @@
<html>
<head>
</head>
<body onload="window.parent.onIframeLoaded()" onunload="window.parent.onIframeUnloaded();">
<body onload="window.parent.onIframeLoaded()">
<p>This frame should be replaced before the test ends</p>
</body>
</html>
@@ -1,3 +1,15 @@
2017-12-06 Chris Dumez <cdumez@apple.com>

ServiceWorkers API should reject promises when calling objects inside detached frames
https://bugs.webkit.org/show_bug.cgi?id=180444

Reviewed by Youenn Fablet.

Rebaseline test now that it is passing some checks.

* web-platform-tests/service-workers/service-worker/detached-context.https-expected.txt:
* web-platform-tests/service-workers/service-worker/register-closed-window.https-expected.txt:

2017-12-06 Youenn Fablet <youenn@apple.com>

Service Worker fetch should filter HTTP headers that are added by CachedResourceLoader/CachedResource
@@ -1,9 +1,8 @@

Harness Error (TIMEOUT), message = null

TIMEOUT accessing a ServiceWorkerRegistration from a removed iframe Test timed out
NOTRUN accessing a ServiceWorker object from a removed iframe
NOTRUN accessing navigator.serviceWorker on a detached iframe
PASS accessing a ServiceWorkerRegistration from a removed iframe
PASS accessing a ServiceWorker object from a removed iframe
PASS accessing navigator.serviceWorker on a detached iframe
FAIL accessing navigator on a removed frame assert_throws: function "() => get_navigator()" did not throw
FAIL accessing navigator.serviceWorker on a removed about:blank frame undefined is not an object (evaluating 'get_navigator().serviceWorker')

@@ -1,3 +1,5 @@
CONSOLE MESSAGE: Unhandled Promise Rejection: InvalidStateError: The object is in an invalid state.
CONSOLE MESSAGE: Unhandled Promise Rejection: InvalidStateError: The object is in an invalid state.

PASS ready returns the same Promise object
PASS ready returns a Promise object in the context of the related document
@@ -1,3 +1,4 @@
CONSOLE MESSAGE: Unhandled Promise Rejection: TypeError: serviceWorker.register() must be called with a valid relative script URL

PASS Call register() on ServiceWorkerContainer owned by closed window.

@@ -1,3 +1,4 @@
CONSOLE MESSAGE: Unhandled Promise Rejection: TypeError: Not enough arguments
This tests that the navigator object of a deleted frame is disconnected properly. Accessing fields or methods shouldn't crash the browser.
Check Navigator
navigator.appCodeName is OK
@@ -1,3 +1,4 @@
CONSOLE MESSAGE: Unhandled Promise Rejection: TypeError: Not enough arguments
This tests that the navigator object of a deleted frame is disconnected properly. Accessing fields or methods shouldn't crash the browser.
Check Navigator
navigator.appCodeName is OK
@@ -1,3 +1,4 @@
CONSOLE MESSAGE: Unhandled Promise Rejection: TypeError: Not enough arguments
This tests that the navigator object of a deleted frame is disconnected properly. Accessing fields or methods shouldn't crash the browser.
Check Navigator
navigator.appCodeName is OK
@@ -1,3 +1,4 @@
CONSOLE MESSAGE: Unhandled Promise Rejection: TypeError: Not enough arguments
This tests that the navigator object of a deleted frame is disconnected properly. Accessing fields or methods shouldn't crash the browser.
Check Navigator
navigator.appCodeName is OK
@@ -1,3 +1,41 @@
2017-12-06 Chris Dumez <cdumez@apple.com>

ServiceWorkers API should reject promises when calling objects inside detached frames
https://bugs.webkit.org/show_bug.cgi?id=180444

Reviewed by Youenn Fablet.

ServiceWorkers API should reject promises when calling objects inside detached frames.

No new tests, rebaselined existing test.

* bindings/js/JSDOMPromiseDeferred.h:
(WebCore::callPromiseFunction):
Use the caller's globalObject instead of the lexicalGlobalObject when constructing the
deferred promise. The bug became visible when working on this service worker bug since
rejecting the promise when the frame is detached did not actually work. The issue is
that since the promise was created with the detached frame's globalObject, then it was
suspended and would not run script.

* bindings/js/JSDOMWindowBase.cpp:
(WebCore::callerGlobalObject):
(WebCore::incumbentDOMWindow):
* bindings/js/JSDOMWindowBase.h:
Add convenience function to get the caller's globalObject. It was carved out of
incumbentDOMWindow().

* workers/service/ServiceWorker.cpp:
(WebCore::ServiceWorker::postMessage):
* workers/service/ServiceWorkerContainer.cpp:
(WebCore::ServiceWorkerContainer::addRegistration):
(WebCore::ServiceWorkerContainer::getRegistration):
(WebCore::ServiceWorkerContainer::getRegistrations):
* workers/service/ServiceWorkerRegistration.cpp:
(WebCore::ServiceWorkerRegistration::update):
(WebCore::ServiceWorkerRegistration::unregister):
Reject the promise when m_isStopped flag is set (i.e. ActiveDOMObject::stop()
has been called).

2017-12-06 Said Abou-Hallawa <sabouhallawa@apple.com>

[Mac] REGRESSION (r224527): Drawing a path with box-shadow takes double the blur-radius on Retina display
@@ -271,4 +271,42 @@ JSDOMGlobalObject* toJSDOMGlobalObject(ScriptExecutionContext* scriptExecutionCo
return nullptr;
}

JSDOMGlobalObject& callerGlobalObject(ExecState& state)
{
class GetCallerGlobalObjectFunctor {
public:
GetCallerGlobalObjectFunctor() = default;

StackVisitor::Status operator()(StackVisitor& visitor) const
{
if (!m_hasSkippedFirstFrame) {
m_hasSkippedFirstFrame = true;
return StackVisitor::Continue;
}

if (auto* codeBlock = visitor->codeBlock())
m_globalObject = codeBlock->globalObject();
else {
ASSERT(visitor->callee().rawPtr());
// FIXME: Callee is not an object if the caller is Web Assembly.
// Figure out what to do here. We can probably get the global object
// from the top-most Wasm Instance. https://bugs.webkit.org/show_bug.cgi?id=165721
if (visitor->callee().isCell() && visitor->callee().asCell()->isObject())
m_globalObject = jsCast<JSObject*>(visitor->callee().asCell())->globalObject();
}
return StackVisitor::Done;
}

JSGlobalObject* globalObject() const { return m_globalObject; }

private:
mutable bool m_hasSkippedFirstFrame { false };
mutable JSGlobalObject* m_globalObject { nullptr };
};

GetCallerGlobalObjectFunctor iter;
state.iterate(iter);
return *jsCast<JSDOMGlobalObject*>(iter.globalObject() ? iter.globalObject() : state.vmEntryGlobalObject());
}

} // namespace WebCore
@@ -129,4 +129,6 @@ JSDOMGlobalObject* toJSDOMGlobalObject(ScriptExecutionContext*, JSC::ExecState*)
JSDOMGlobalObject* toJSDOMGlobalObject(Document*, DOMWrapperWorld&);
JSDOMGlobalObject* toJSDOMGlobalObject(ScriptExecutionContext*, DOMWrapperWorld&);

WEBCORE_EXPORT JSDOMGlobalObject& callerGlobalObject(JSC::ExecState&);

} // namespace WebCore
@@ -264,7 +264,7 @@ inline JSC::JSValue callPromiseFunction(JSC::ExecState& state)
JSC::VM& vm = state.vm();
auto scope = DECLARE_CATCH_SCOPE(vm);

JSDOMGlobalObject& globalObject = *JSC::jsCast<JSDOMGlobalObject*>(state.lexicalGlobalObject());
auto& globalObject = callerGlobalObject(state);
JSC::JSPromiseDeferred* promiseDeferred = JSC::JSPromiseDeferred::create(&state, &globalObject);

// promiseDeferred can be null when terminating a Worker abruptly.
@@ -284,7 +284,7 @@ inline JSC::JSValue callPromiseFunction(JSC::ExecState& state, PromiseFunctor fu
JSC::VM& vm = state.vm();
auto scope = DECLARE_CATCH_SCOPE(vm);

JSDOMGlobalObject& globalObject = *JSC::jsCast<JSDOMGlobalObject*>(state.lexicalGlobalObject());
auto& globalObject = callerGlobalObject(state);
JSC::JSPromiseDeferred* promiseDeferred = JSC::JSPromiseDeferred::create(&state, &globalObject);

// promiseDeferred can be null when terminating a Worker abruptly.
@@ -289,40 +289,7 @@ JSDOMWindow* toJSDOMWindow(JSC::VM& vm, JSValue value)

DOMWindow& incumbentDOMWindow(ExecState& state)
{
class GetCallerGlobalObjectFunctor {
public:
GetCallerGlobalObjectFunctor() = default;

StackVisitor::Status operator()(StackVisitor& visitor) const
{
if (!m_hasSkippedFirstFrame) {
m_hasSkippedFirstFrame = true;
return StackVisitor::Continue;
}

if (auto* codeBlock = visitor->codeBlock())
m_globalObject = codeBlock->globalObject();
else {
ASSERT(visitor->callee().rawPtr());
// FIXME: Callee is not an object if the caller is Web Assembly.
// Figure out what to do here. We can probably get the global object
// from the top-most Wasm Instance. https://bugs.webkit.org/show_bug.cgi?id=165721
if (visitor->callee().isCell() && visitor->callee().asCell()->isObject())
m_globalObject = jsCast<JSObject*>(visitor->callee().asCell())->globalObject();
}
return StackVisitor::Done;
}

JSGlobalObject* globalObject() const { return m_globalObject; }

private:
mutable bool m_hasSkippedFirstFrame { false };
mutable JSGlobalObject* m_globalObject { nullptr };
};

GetCallerGlobalObjectFunctor iter;
state.iterate(iter);
return iter.globalObject() ? asJSDOMWindow(iter.globalObject())->wrapped() : firstDOMWindow(state);
return asJSDOMWindow(&callerGlobalObject(state))->wrapped();
}

DOMWindow& activeDOMWindow(ExecState& state)
@@ -88,6 +88,9 @@ void ServiceWorker::scheduleTaskToUpdateState(State state)

ExceptionOr<void> ServiceWorker::postMessage(ScriptExecutionContext& context, JSC::JSValue messageValue, Vector<JSC::Strong<JSC::JSObject>>&& transfer)
{
if (m_isStopped)
return Exception { InvalidStateError };

if (state() == State::Redundant)
return Exception { InvalidStateError, ASCIILiteral("Service Worker state is redundant") };

@@ -111,8 +111,7 @@ ServiceWorker* ServiceWorkerContainer::controller() const
void ServiceWorkerContainer::addRegistration(const String& relativeScriptURL, const RegistrationOptions& options, Ref<DeferredPromise>&& promise)
{
auto* context = scriptExecutionContext();
if (!context || !context->sessionID().isValid()) {
ASSERT_NOT_REACHED();
if (m_isStopped || !context->sessionID().isValid()) {
promise->reject(Exception(InvalidStateError));
return;
}
@@ -234,15 +233,16 @@ void ServiceWorkerContainer::scheduleJob(Ref<ServiceWorkerJob>&& job)

void ServiceWorkerContainer::getRegistration(const String& clientURL, Ref<DeferredPromise>&& promise)
{
auto* context = scriptExecutionContext();
if (!context) {
ASSERT_NOT_REACHED();
if (m_isStopped) {
promise->reject(Exception { InvalidStateError });
return;
}

URL parsedURL = context->completeURL(clientURL);
if (!protocolHostAndPortAreEqual(parsedURL, context->url())) {
ASSERT(scriptExecutionContext());
auto& context = *scriptExecutionContext();

URL parsedURL = context.completeURL(clientURL);
if (!protocolHostAndPortAreEqual(parsedURL, context.url())) {
promise->reject(Exception { SecurityError, ASCIILiteral("Origin of clientURL is not client's origin") });
return;
}
@@ -252,7 +252,7 @@ void ServiceWorkerContainer::getRegistration(const String& clientURL, Ref<Deferr
m_pendingPromises.add(pendingPromiseIdentifier, WTFMove(pendingPromise));

auto contextIdentifier = this->contextIdentifier();
callOnMainThread([connection = makeRef(ensureSWClientConnection()), this, topOrigin = context->topOrigin().isolatedCopy(), parsedURL = parsedURL.isolatedCopy(), contextIdentifier, pendingPromiseIdentifier]() mutable {
callOnMainThread([connection = makeRef(ensureSWClientConnection()), this, topOrigin = context.topOrigin().isolatedCopy(), parsedURL = parsedURL.isolatedCopy(), contextIdentifier, pendingPromiseIdentifier]() mutable {
connection->matchRegistration(topOrigin, parsedURL, [this, contextIdentifier, pendingPromiseIdentifier] (auto&& result) mutable {
ScriptExecutionContext::postTaskTo(contextIdentifier, [this, pendingPromiseIdentifier, result = crossThreadCopy(result)](ScriptExecutionContext&) mutable {
didFinishGetRegistrationRequest(pendingPromiseIdentifier, WTFMove(result));
@@ -300,19 +300,21 @@ void ServiceWorkerContainer::scheduleTaskToUpdateRegistrationState(ServiceWorker

void ServiceWorkerContainer::getRegistrations(Ref<DeferredPromise>&& promise)
{
auto* context = scriptExecutionContext();
if (!context) {
if (m_isStopped) {
promise->reject(Exception { InvalidStateError });
return;
}

ASSERT(scriptExecutionContext());
auto& context = *scriptExecutionContext();

uint64_t pendingPromiseIdentifier = ++m_lastPendingPromiseIdentifier;
auto pendingPromise = std::make_unique<PendingPromise>(WTFMove(promise), makePendingActivity(*this));
m_pendingPromises.add(pendingPromiseIdentifier, WTFMove(pendingPromise));

auto contextIdentifier = this->contextIdentifier();
auto contextURL = context->url();
callOnMainThread([connection = makeRef(ensureSWClientConnection()), this, topOrigin = context->topOrigin().isolatedCopy(), contextURL = contextURL.isolatedCopy(), contextIdentifier, pendingPromiseIdentifier]() mutable {
auto contextURL = context.url();
callOnMainThread([connection = makeRef(ensureSWClientConnection()), this, topOrigin = context.topOrigin().isolatedCopy(), contextURL = contextURL.isolatedCopy(), contextIdentifier, pendingPromiseIdentifier]() mutable {
connection->getRegistrations(topOrigin, contextURL, [this, contextIdentifier, pendingPromiseIdentifier] (auto&& registrationDatas) mutable {
ScriptExecutionContext::postTaskTo(contextIdentifier, [this, pendingPromiseIdentifier, registrationDatas = crossThreadCopy(registrationDatas)](ScriptExecutionContext&) mutable {
didFinishGetRegistrationsRequest(pendingPromiseIdentifier, WTFMove(registrationDatas));

0 comments on commit 12f5d12

Please sign in to comment.