Changeset 225577 in webkit


Ignore:
Timestamp:
Dec 6, 2017 10:40:05 AM (6 years ago)
Author:
Chris Dumez
Message:

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:

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.

Location:
trunk
Files:
24 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r225576 r225577  
     12017-12-06  Chris Dumez  <cdumez@apple.com>
     2
     3        ServiceWorkers API should reject promises when calling objects inside detached frames
     4        https://bugs.webkit.org/show_bug.cgi?id=180444
     5
     6        Reviewed by Youenn Fablet.
     7
     8        * TestExpectations:
     9        Unskip test that no longer times out and starts passing a few checks.
     10
     11        * fast/dom/navigator-detached-no-crash-expected.txt:
     12        Rebaseline test now that promise is rejected.
     13
     14        * http/tests/media/media-stream/disconnected-frame-permission-denied-expected.txt:
     15        * http/tests/media/media-stream/disconnected-frame-permission-denied.html:
     16        Update and rebaseline test now that the promise is rejected. I verified that this
     17        behavior is consistent with Chrome.
     18
    1192017-12-06  Matt Lewis  <jlewis3@apple.com>
    220

  • trunk/LayoutTests/TestExpectations

    r225566 r225577  
    146146# Skip service worker tests that are timing out.
    147147imported/w3c/web-platform-tests/fetch/api/abort/general-serviceworker.https.html [ Skip ]
    148 imported/w3c/web-platform-tests/service-workers/service-worker/detached-context.https.html [ Skip ]
    149148imported/w3c/web-platform-tests/service-workers/service-worker/extendable-event-waituntil.https.html [ Skip ]
    150149imported/w3c/web-platform-tests/service-workers/service-worker/fetch-event-respond-with-partial-stream.https.html [ Skip ]

  • trunk/LayoutTests/fast/dom/navigator-detached-no-crash-expected.txt

    r222958 r225577  
     1CONSOLE MESSAGE: Unhandled Promise Rejection: TypeError: Not enough arguments
    12This tests that the navigator object of a deleted frame is disconnected properly. Accessing fields or methods shouldn't crash the browser.
    23 Check Navigator

  • trunk/LayoutTests/http/tests/media/media-stream/disconnected-frame-expected.txt

    r176011 r225577  
    1 frame "<!--framePath //<!--frame0-->-->" - has 1 onunload handler(s)
    21Tests 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.
    32

  • trunk/LayoutTests/http/tests/media/media-stream/disconnected-frame-permission-denied-expected.txt

    r201080 r225577  
    1 frame "<!--framePath //<!--frame0-->-->" - has 1 onunload handler(s)
    21Tests 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.
    32
    … …  
    76PASS error.name is "NotAllowedError"
    87
    9 PASS No callbacks invoked
     8PASS Error callback invoked
    109PASS successfullyParsed is true
    1110

  • trunk/LayoutTests/http/tests/media/media-stream/disconnected-frame-permission-denied.html

    r211962 r225577  
    1818function onIframeLoaded() {
    1919    iframeNavigator = iframe.contentWindow.navigator;
     20    iframe.contentWindow.onunload = onIframeUnloaded;
    2021    iframeNavigator.mediaDevices.getUserMedia(options)
    2122        .then(stream => {
    … …  
    3940        })
    4041        .catch(err => {
    41             testFailed('Error callback invoked unexpectedly');
     42            testPassed('Error callback invoked');
    4243            finishJSTest();
    4344        });
    4445    setTimeout(function() {
    45         testPassed('No callbacks invoked');
     46        testFailed('No callbacks invoked');
    4647        finishJSTest();
    4748    }, 100);

  • trunk/LayoutTests/http/tests/media/media-stream/disconnected-frame.html

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

  • trunk/LayoutTests/http/tests/media/media-stream/resources/disconnected-frame-inner.html

    r176011 r225577  
    33  <head>
    44  </head>
    5   <body onload="window.parent.onIframeLoaded()" onunload="window.parent.onIframeUnloaded();">
     5  <body onload="window.parent.onIframeLoaded()">
    66    <p>This frame should be replaced before the test ends</p>
    77  </body>

  • trunk/LayoutTests/imported/w3c/ChangeLog

    r225574 r225577  
     12017-12-06  Chris Dumez  <cdumez@apple.com>
     2
     3        ServiceWorkers API should reject promises when calling objects inside detached frames
     4        https://bugs.webkit.org/show_bug.cgi?id=180444
     5
     6        Reviewed by Youenn Fablet.
     7
     8        Rebaseline test now that it is passing some checks.
     9
     10        * web-platform-tests/service-workers/service-worker/detached-context.https-expected.txt:
     11        * web-platform-tests/service-workers/service-worker/register-closed-window.https-expected.txt:
     12
    1132017-12-06  Youenn Fablet  <youenn@apple.com>
    214

  • trunk/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/detached-context.https-expected.txt

    r224730 r225577  
    11
    2 Harness Error (TIMEOUT), message = null
    32
    4 TIMEOUT accessing a ServiceWorkerRegistration from a removed iframe Test timed out
    5 NOTRUN accessing a ServiceWorker object from a removed iframe
    6 NOTRUN accessing navigator.serviceWorker on a detached iframe
     3PASS accessing a ServiceWorkerRegistration from a removed iframe
     4PASS accessing a ServiceWorker object from a removed iframe
     5PASS accessing navigator.serviceWorker on a detached iframe
    76FAIL accessing navigator on a removed frame assert_throws: function "() => get_navigator()" did not throw
    87FAIL accessing navigator.serviceWorker on a removed about:blank frame undefined is not an object (evaluating 'get_navigator().serviceWorker')

  • trunk/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/ready.https-expected.txt

    r225531 r225577  
     1CONSOLE MESSAGE: Unhandled Promise Rejection: InvalidStateError: The object is in an invalid state.
     2CONSOLE MESSAGE: Unhandled Promise Rejection: InvalidStateError: The object is in an invalid state.
    13
    24PASS ready returns the same Promise object

  • trunk/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/register-closed-window.https-expected.txt

    r224132 r225577  
     1CONSOLE MESSAGE: Unhandled Promise Rejection: TypeError: serviceWorker.register() must be called with a valid relative script URL
    12
    23PASS Call register() on ServiceWorkerContainer owned by closed window.

  • trunk/LayoutTests/platform/gtk/fast/dom/navigator-detached-no-crash-expected.txt

    r219403 r225577  
     1CONSOLE MESSAGE: Unhandled Promise Rejection: TypeError: Not enough arguments
    12This tests that the navigator object of a deleted frame is disconnected properly. Accessing fields or methods shouldn't crash the browser.
    23 Check Navigator

  • trunk/LayoutTests/platform/mac-elcapitan-wk2/fast/dom/navigator-detached-no-crash-expected.txt

    r222958 r225577  
     1CONSOLE MESSAGE: Unhandled Promise Rejection: TypeError: Not enough arguments
    12This tests that the navigator object of a deleted frame is disconnected properly. Accessing fields or methods shouldn't crash the browser.
    23 Check Navigator

  • trunk/LayoutTests/platform/mac-wk1/fast/dom/navigator-detached-no-crash-expected.txt

    r224833 r225577  
     1CONSOLE MESSAGE: Unhandled Promise Rejection: TypeError: Not enough arguments
    12This tests that the navigator object of a deleted frame is disconnected properly. Accessing fields or methods shouldn't crash the browser.
    23 Check Navigator

  • trunk/LayoutTests/platform/win/fast/dom/navigator-detached-no-crash-expected.txt

    r220507 r225577  
     1CONSOLE MESSAGE: Unhandled Promise Rejection: TypeError: Not enough arguments
    12This tests that the navigator object of a deleted frame is disconnected properly. Accessing fields or methods shouldn't crash the browser.
    23 Check Navigator

  • trunk/Source/WebCore/ChangeLog

    r225575 r225577  
     12017-12-06  Chris Dumez  <cdumez@apple.com>
     2
     3        ServiceWorkers API should reject promises when calling objects inside detached frames
     4        https://bugs.webkit.org/show_bug.cgi?id=180444
     5
     6        Reviewed by Youenn Fablet.
     7
     8        ServiceWorkers API should reject promises when calling objects inside detached frames.
     9
     10        No new tests, rebaselined existing test.
     11
     12        * bindings/js/JSDOMPromiseDeferred.h:
     13        (WebCore::callPromiseFunction):
     14        Use the caller's globalObject instead of the lexicalGlobalObject when constructing the
     15        deferred promise. The bug became visible when working on this service worker bug since
     16        rejecting the promise when the frame is detached did not actually work. The issue is
     17        that since the promise was created with the detached frame's globalObject, then it was
     18        suspended and would not run script.
     19
     20        * bindings/js/JSDOMWindowBase.cpp:
     21        (WebCore::callerGlobalObject):
     22        (WebCore::incumbentDOMWindow):
     23        * bindings/js/JSDOMWindowBase.h:
     24        Add convenience function to get the caller's globalObject. It was carved out of
     25        incumbentDOMWindow().
     26
     27        * workers/service/ServiceWorker.cpp:
     28        (WebCore::ServiceWorker::postMessage):
     29        * workers/service/ServiceWorkerContainer.cpp:
     30        (WebCore::ServiceWorkerContainer::addRegistration):
     31        (WebCore::ServiceWorkerContainer::getRegistration):
     32        (WebCore::ServiceWorkerContainer::getRegistrations):
     33        * workers/service/ServiceWorkerRegistration.cpp:
     34        (WebCore::ServiceWorkerRegistration::update):
     35        (WebCore::ServiceWorkerRegistration::unregister):
     36        Reject the promise when m_isStopped flag is set (i.e. ActiveDOMObject::stop()
     37        has been called).
     38
    1392017-12-06  Said Abou-Hallawa  <sabouhallawa@apple.com>
    240

  • trunk/Source/WebCore/bindings/js/JSDOMGlobalObject.cpp

    r223728 r225577  
    272272}
    273273
     274JSDOMGlobalObject& callerGlobalObject(ExecState& state)
     275{
     276    class GetCallerGlobalObjectFunctor {
     277    public:
     278        GetCallerGlobalObjectFunctor() = default;
     279
     280        StackVisitor::Status operator()(StackVisitor& visitor) const
     281        {
     282            if (!m_hasSkippedFirstFrame) {
     283                m_hasSkippedFirstFrame = true;
     284                return StackVisitor::Continue;
     285            }
     286
     287            if (auto* codeBlock = visitor->codeBlock())
     288                m_globalObject = codeBlock->globalObject();
     289            else {
     290                ASSERT(visitor->callee().rawPtr());
     291                // FIXME: Callee is not an object if the caller is Web Assembly.
     292                // Figure out what to do here. We can probably get the global object
     293                // from the top-most Wasm Instance. https://bugs.webkit.org/show_bug.cgi?id=165721
     294                if (visitor->callee().isCell() && visitor->callee().asCell()->isObject())
     295                    m_globalObject = jsCast<JSObject*>(visitor->callee().asCell())->globalObject();
     296            }
     297            return StackVisitor::Done;
     298        }
     299
     300        JSGlobalObject* globalObject() const { return m_globalObject; }
     301
     302    private:
     303        mutable bool m_hasSkippedFirstFrame { false };
     304        mutable JSGlobalObject* m_globalObject { nullptr };
     305    };
     306
     307    GetCallerGlobalObjectFunctor iter;
     308    state.iterate(iter);
     309    return *jsCast<JSDOMGlobalObject*>(iter.globalObject() ? iter.globalObject() : state.vmEntryGlobalObject());
     310}
     311
    274312} // namespace WebCore

  • trunk/Source/WebCore/bindings/js/JSDOMGlobalObject.h

    r218342 r225577  
    130130JSDOMGlobalObject* toJSDOMGlobalObject(ScriptExecutionContext*, DOMWrapperWorld&);
    131131
     132WEBCORE_EXPORT JSDOMGlobalObject& callerGlobalObject(JSC::ExecState&);
     133
    132134} // namespace WebCore

  • trunk/Source/WebCore/bindings/js/JSDOMPromiseDeferred.h

    r221849 r225577  
    265265    auto scope = DECLARE_CATCH_SCOPE(vm);
    266266
    267     JSDOMGlobalObject& globalObject = *JSC::jsCast<JSDOMGlobalObject*>(state.lexicalGlobalObject());
     267    auto& globalObject = callerGlobalObject(state);
    268268    JSC::JSPromiseDeferred* promiseDeferred = JSC::JSPromiseDeferred::create(&state, &globalObject);
    269269
    … …  
    285285    auto scope = DECLARE_CATCH_SCOPE(vm);
    286286
    287     JSDOMGlobalObject& globalObject = *JSC::jsCast<JSDOMGlobalObject*>(state.lexicalGlobalObject());
     287    auto& globalObject = callerGlobalObject(state);
    288288    JSC::JSPromiseDeferred* promiseDeferred = JSC::JSPromiseDeferred::create(&state, &globalObject);
    289289

  • trunk/Source/WebCore/bindings/js/JSDOMWindowBase.cpp

    r224736 r225577  
    290290DOMWindow& incumbentDOMWindow(ExecState& state)
    291291{
    292     class GetCallerGlobalObjectFunctor {
    293     public:
    294         GetCallerGlobalObjectFunctor() = default;
    295 
    296         StackVisitor::Status operator()(StackVisitor& visitor) const
    297         {
    298             if (!m_hasSkippedFirstFrame) {
    299                 m_hasSkippedFirstFrame = true;
    300                 return StackVisitor::Continue;
    301             }
    302 
    303             if (auto* codeBlock = visitor->codeBlock())
    304                 m_globalObject = codeBlock->globalObject();
    305             else {
    306                 ASSERT(visitor->callee().rawPtr());
    307                 // FIXME: Callee is not an object if the caller is Web Assembly.
    308                 // Figure out what to do here. We can probably get the global object
    309                 // from the top-most Wasm Instance. https://bugs.webkit.org/show_bug.cgi?id=165721
    310                 if (visitor->callee().isCell() && visitor->callee().asCell()->isObject())
    311                     m_globalObject = jsCast<JSObject*>(visitor->callee().asCell())->globalObject();
    312             }
    313             return StackVisitor::Done;
    314         }
    315 
    316         JSGlobalObject* globalObject() const { return m_globalObject; }
    317 
    318     private:
    319         mutable bool m_hasSkippedFirstFrame { false };
    320         mutable JSGlobalObject* m_globalObject { nullptr };
    321     };
    322 
    323     GetCallerGlobalObjectFunctor iter;
    324     state.iterate(iter);
    325     return iter.globalObject() ? asJSDOMWindow(iter.globalObject())->wrapped() : firstDOMWindow(state);
     292    return asJSDOMWindow(&callerGlobalObject(state))->wrapped();
    326293}
    327294

  • trunk/Source/WebCore/workers/service/ServiceWorker.cpp

    r225462 r225577  
    8989ExceptionOr<void> ServiceWorker::postMessage(ScriptExecutionContext& context, JSC::JSValue messageValue, Vector<JSC::Strong<JSC::JSObject>>&& transfer)
    9090{
     91    if (m_isStopped)
     92        return Exception { InvalidStateError };
     93
    9194    if (state() == State::Redundant)
    9295        return Exception { InvalidStateError, ASCIILiteral("Service Worker state is redundant") };

  • trunk/Source/WebCore/workers/service/ServiceWorkerContainer.cpp

    r225531 r225577  
    112112{
    113113    auto* context = scriptExecutionContext();
    114     if (!context || !context->sessionID().isValid()) {
    115         ASSERT_NOT_REACHED();
     114    if (m_isStopped || !context->sessionID().isValid()) {
    116115        promise->reject(Exception(InvalidStateError));
    117116        return;
    … …  
    235234void ServiceWorkerContainer::getRegistration(const String& clientURL, Ref<DeferredPromise>&& promise)
    236235{
    237     auto* context = scriptExecutionContext();
    238     if (!context) {
    239         ASSERT_NOT_REACHED();
     236    if (m_isStopped) {
    240237        promise->reject(Exception { InvalidStateError });
    241238        return;
    242239    }
    243240
    244     URL parsedURL = context->completeURL(clientURL);
    245     if (!protocolHostAndPortAreEqual(parsedURL, context->url())) {
     241    ASSERT(scriptExecutionContext());
     242    auto& context = *scriptExecutionContext();
     243
     244    URL parsedURL = context.completeURL(clientURL);
     245    if (!protocolHostAndPortAreEqual(parsedURL, context.url())) {
    246246        promise->reject(Exception { SecurityError, ASCIILiteral("Origin of clientURL is not client's origin") });
    247247        return;
    … …  
    253253
    254254    auto contextIdentifier = this->contextIdentifier();
    255     callOnMainThread([connection = makeRef(ensureSWClientConnection()), this, topOrigin = context->topOrigin().isolatedCopy(), parsedURL = parsedURL.isolatedCopy(), contextIdentifier, pendingPromiseIdentifier]() mutable {
     255    callOnMainThread([connection = makeRef(ensureSWClientConnection()), this, topOrigin = context.topOrigin().isolatedCopy(), parsedURL = parsedURL.isolatedCopy(), contextIdentifier, pendingPromiseIdentifier]() mutable {
    256256        connection->matchRegistration(topOrigin, parsedURL, [this, contextIdentifier, pendingPromiseIdentifier] (auto&& result) mutable {
    257257            ScriptExecutionContext::postTaskTo(contextIdentifier, [this, pendingPromiseIdentifier, result = crossThreadCopy(result)](ScriptExecutionContext&) mutable {
    … …  
    301301void ServiceWorkerContainer::getRegistrations(Ref<DeferredPromise>&& promise)
    302302{
    303     auto* context = scriptExecutionContext();
    304     if (!context) {
     303    if (m_isStopped) {
    305304        promise->reject(Exception { InvalidStateError });
    306305        return;
    307306    }
     307
     308    ASSERT(scriptExecutionContext());
     309    auto& context = *scriptExecutionContext();
    308310
    309311    uint64_t pendingPromiseIdentifier = ++m_lastPendingPromiseIdentifier;
    … …  
    312314
    313315    auto contextIdentifier = this->contextIdentifier();
    314     auto contextURL = context->url();
    315     callOnMainThread([connection = makeRef(ensureSWClientConnection()), this, topOrigin = context->topOrigin().isolatedCopy(), contextURL = contextURL.isolatedCopy(), contextIdentifier, pendingPromiseIdentifier]() mutable {
     316    auto contextURL = context.url();
     317    callOnMainThread([connection = makeRef(ensureSWClientConnection()), this, topOrigin = context.topOrigin().isolatedCopy(), contextURL = contextURL.isolatedCopy(), contextIdentifier, pendingPromiseIdentifier]() mutable {
    316318        connection->getRegistrations(topOrigin, contextURL, [this, contextIdentifier, pendingPromiseIdentifier] (auto&& registrationDatas) mutable {
    317319            ScriptExecutionContext::postTaskTo(contextIdentifier, [this, pendingPromiseIdentifier, registrationDatas = crossThreadCopy(registrationDatas)](ScriptExecutionContext&) mutable {

  • trunk/Source/WebCore/workers/service/ServiceWorkerRegistration.cpp

    r225456 r225577  
    115115void ServiceWorkerRegistration::update(Ref<DeferredPromise>&& promise)
    116116{
    117     auto* context = scriptExecutionContext();
    118     if (!context) {
     117    if (m_isStopped) {
    119118        promise->reject(Exception(InvalidStateError));
    120119        return;
    … …  
    133132void ServiceWorkerRegistration::unregister(Ref<DeferredPromise>&& promise)
    134133{
    135     auto* context = scriptExecutionContext();
    136     if (!context) {
     134    if (m_isStopped) {
    137135        promise->reject(Exception(InvalidStateError));
    138136        return;
Note: See TracChangeset for help on using the changeset viewer.