Use caller's document url to resolve scriptURL/patternURL in registerServiceWorker/getRegistration

Source/modules/serviceworkers/ServiceWorkerContainer.cpp

Index: Source/modules/serviceworkers/ServiceWorkerContainer.cpp
diff --git a/Source/modules/serviceworkers/ServiceWorkerContainer.cpp b/Source/modules/serviceworkers/ServiceWorkerContainer.cpp
index 5bbf765d400c68d6259cd439aba44e8680e1c863..cfcb4ff88dcea8fd5bf6aea7d27b744c4783e9a5 100644
--- a/Source/modules/serviceworkers/ServiceWorkerContainer.cpp
+++ b/Source/modules/serviceworkers/ServiceWorkerContainer.cpp
@@ -117,8 +117,6 @@ ScriptPromise ServiceWorkerContainer::registerServiceWorker(ScriptState* scriptS
         return promise;
     }
 
-    // FIXME: This should use the container's execution context, not
-    // the callers.
     ExecutionContext* executionContext = scriptState->executionContext();

[dominicc (has gone to gerrit), 2015/02/09 07:31:48]

I'm still not sure this is the right context. Which context is this? For example
if in frame A you do

frameB.contentWindow.ServiceWorkerContainer.prototype.register.call(frameC.navigator.serviceWorker,
...)

which one is used? (I think that the one we want is "C", BTW.)

[jungkees, 2015/02/13 08:12:37]

Having double-checked it, it's basically the container's context (not the
caller's context). I.e. for
frameB.contentWindow.navigator.serviceWorker.register(), frameB's context is
being used.

For the JS function's call method case as in your example, frameB's context is
also used, though. The ScriptState object (related to the context) is set to the
isolate's current context in the auto generated V8 binding code like the
following: 

ScriptState* scriptState = ScriptState::current(info.GetIsolate());
ScriptPromise result = impl->registerServiceWorker(scriptState, url, options);

If frameC's context is expected, do you think we better file a separate issue
for this?

     RefPtr<SecurityOrigin> documentOrigin = executionContext->securityOrigin();
     String errorMessage;
@@ -133,14 +131,16 @@ ScriptPromise ServiceWorkerContainer::registerServiceWorker(ScriptState* scriptS
         return promise;
     }
 
-    KURL patternURL = executionContext->completeURL(options.scope());
+    Document* callingDocument = callingDOMWindow(scriptState->isolate())->document();
+
+    KURL patternURL = callingDocument ? callingDocument->completeURL(options.scope()) : executionContext->completeURL(options.scope());

[dominicc (has gone to gerrit), 2015/02/09 07:31:48]

I don't think this in right; I think if from frame A we do

frameB.contentWindow.navigator.serviceWorker.register

then the URL should be resolved against frame B's location. For example look at
how LocalDOMWindow::open (which corresponds to Window.open) uses the entered
document to complete the URL, not the calling document. I'd be surprised if
register worked differently, but maybe you can point me to specs? Am I missing
something?

[jungkees, 2015/02/13 08:12:38]

This is not register specific, but HTML specifies this. I think this pointer
explains:
https://html.spec.whatwg.org/multipage/webappapis.html#entry-settings-object.
See the example snippet just below the definition of entry settings object and
incumbent settings objects.

[dominicc (has gone to gerrit), 2015/02/16 06:54:45]

On 2015/02/13 at 08:12:38, jungkees wrote: 
Thanks for the link, that does look like the definitive thing.

Where is the spec for using the current execution context when the calling
document is null? What's the precedent for that?

[jungkees, 2015/02/16 09:06:10]

My intention with checking whether the callingDocument be null was just to set a
guard to not dereference the value when null is returned. So did I assume the
callingDocument in this code refers to the iframe's document when it exists. If
a document doesn't have any iframe and the method is called withing the context,
shouldn't it use the current execution context by default? Please let me know if
I'm missing or overlooking any basic stuff here.

     patternURL.removeFragmentIdentifier();
     if (!documentOrigin->canRequest(patternURL)) {
         resolver->reject(DOMException::create(SecurityError, "The scope must match the current origin."));
         return promise;
     }
 
-    KURL scriptURL = executionContext->completeURL(url);
+    KURL scriptURL = callingDocument ? callingDocument->completeURL(url) : executionContext->completeURL(url);
     scriptURL.removeFragmentIdentifier();
     if (!documentOrigin->canRequest(scriptURL)) {
         resolver->reject(DOMException::create(SecurityError, "The origin of the script must match the current origin."));
@@ -181,8 +181,6 @@ ScriptPromise ServiceWorkerContainer::getRegistration(ScriptState* scriptState,
         return promise;
     }
 
-    // FIXME: This should use the container's execution context, not
-    // the callers.
     ExecutionContext* executionContext = scriptState->executionContext();
     RefPtr<SecurityOrigin> documentOrigin = executionContext->securityOrigin();
     String errorMessage;
@@ -197,7 +195,8 @@ ScriptPromise ServiceWorkerContainer::getRegistration(ScriptState* scriptState,
         return promise;
     }
 
-    KURL completedURL = executionContext->completeURL(documentURL);
+    Document* callingDocument = callingDOMWindow(scriptState->isolate())->document();
+    KURL completedURL = callingDocument ? callingDocument->completeURL(documentURL) : executionContext->completeURL(documentURL);
     completedURL.removeFragmentIdentifier();
     if (!documentOrigin->canRequest(completedURL)) {
         resolver->reject(DOMException::create(SecurityError, "The documentURL must match the current origin."));