1218148 - Implement WindowClient.navigate()

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Comment 1

[Andrew Overholt [:overholt]]

Looks like we may already have docs (if not an implementation), Jean-Yves: https://developer.mozilla.org/en-US/docs/Web/API/WindowClient/navigate

Comment 2

[Andreas Farre [:farre]]

Attachment: Bug 1218148 - Implement WindowClient.navigate()

Review commit: https://reviewboard.mozilla.org/r/58520/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58520/

Comment 3

[Andreas Farre [:farre]]

Attachment: Bug 1218148 - Web Platform tests for WindowClient.navigate()

Review commit: https://reviewboard.mozilla.org/r/58522/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58522/

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 8761222 [details]
Bug 1218148 - Implement WindowClient.navigate()

https://reviewboard.mozilla.org/r/58520/#review55636

It looks good! The only thing I would like to see improved is this ResolveOrRejectPromiseRunnable.
Currently it doing too many things and it's hard to follow them via CTORs.
What about if:

if mRV is error => reject with error.
otherwise if client == null -> resolve with null
otherwise -> resolve with the client

And we make 2 CTORs:

with aRV: and MOZ_ASSERT(NS_FAILED(aRv));
with Client: and it can be null. Add a comment about it.

no mNullResolve (because we can use !mClient).

::: dom/workers/ServiceWorkerWindowClient.cpp:75
(Diff revision 1)
> +  ResolveOrRejectPromiseRunnable(WorkerPrivate* aWorkerPrivate,
> +                                 PromiseWorkerProxy* aPromiseProxy)
> +    : WorkerRunnable(aWorkerPrivate, WorkerThreadModifyBusyCount)
> +    , mPromiseProxy(aPromiseProxy)
> +    , mClientInfo(nullptr)
> +    , mNullResolve(true)

What's the default value for mRV here?

::: dom/workers/ServiceWorkerWindowClient.cpp:87
(Diff revision 1)
> +                                 nsresult aRv)
> +    : WorkerRunnable(aWorkerPrivate, WorkerThreadModifyBusyCount)
> +    , mPromiseProxy(aPromiseProxy)
> +    , mClientInfo(nullptr)
> +    , mRv(aRv)
> +    , mNullResolve(false)

This is quite confusing. What about if you do:

ResolveOrRejectPromiseRunnable(..., nsresult aRv, bool aNullResolve) ?

::: dom/workers/ServiceWorkerWindowClient.cpp:240
(Diff revision 1)
> +      return NS_OK;
> +    }
> +
> +    aWebProgress->RemoveProgressListener(this);
> +
> +    MutexAutoLock lock(mPromiseProxy->Lock());

Also here {}

::: dom/workers/ServiceWorkerWindowClient.cpp:262
(Diff revision 1)
> +        clientInfo.reset(new ServiceWorkerClientInfo(doc));
> +        resolveRunnable = new ResolveOrRejectPromiseRunnable(
> +          mPromiseProxy->GetWorkerPrivate(), mPromiseProxy, Move(clientInfo));
> +      } else {
> +        resolveRunnable = new ResolveOrRejectPromiseRunnable(
> +          mPromiseProxy->GetWorkerPrivate(), mPromiseProxy);

It's very confusing the fact if you don't pass arguments, then we resolve it as null.

::: dom/workers/ServiceWorkerWindowClient.cpp:349
(Diff revision 1)
> +  NS_IMETHOD
> +  Run() override
> +  {
> +    AssertIsOnMainThread();
> +
> +    MutexAutoLock lock(mPromiseProxy->Lock());

put this into {} so that you don't lock mProxyProxy for the all Run().

::: dom/workers/ServiceWorkerWindowClient.cpp:354
(Diff revision 1)
> +    MutexAutoLock lock(mPromiseProxy->Lock());
> +    if (mPromiseProxy->CleanedUp()) {
> +      return NS_OK;
> +    }
> +
> +    nsresult rv = Navigate();

Do a quick return here:

if (NS_FAILED(rv)) {
  RejectPromise(rv);
  return;
}

where RejectPromise is:

RefPtr<ResolveOrRejectPromiseRunnable> runnable = new ResolveOrRejectPromiseRunnable(...., rv);
runnable->Dispatch();

::: dom/workers/ServiceWorkerWindowClient.cpp:366
(Diff revision 1)
> +      nsresult rv = NS_NewURI(getter_AddRefs(baseURI), info.mHref);
> +      if (NS_WARN_IF(NS_FAILED(rv))) {
> +        return NS_ERROR_FAILURE;
> +      }
> +
> +      nsGlobalWindow* window = nsGlobalWindow::GetInnerWindowWithId(mWindowId);

Don't do this step twice. Store mWindow as argument. Or return it from Navigate().

::: dom/workers/ServiceWorkerWindowClient.cpp:387
(Diff revision 1)
> +
> +    RefPtr<ResolveOrRejectPromiseRunnable> resolveRunnable;
> +    if (rv != NS_ERROR_DOM_BAD_URI) {
> +      resolveRunnable = new ResolveOrRejectPromiseRunnable(
> +        mPromiseProxy->GetWorkerPrivate(), mPromiseProxy, rv);
> +    } else {

This would be:

new ResolveOrRjectPromiseRunnable(..., rv, rv == NS_ERROR_DOM_BAD_URI)

::: dom/workers/ServiceWorkerWindowClient.cpp:407
(Diff revision 1)
> +    WorkerPrivate::LocationInfo& info = workerPrivate->GetLocationInfo();
> +
> +    nsCOMPtr<nsIURI> baseUrl;
> +    nsresult rv = NS_NewURI(getter_AddRefs(baseUrl), info.mHref);
> +    if (NS_FAILED(rv)) {
> +      return NS_ERROR_FAILURE;

if (NS_WARN_IF(NS_FAILED(rv))) {
  return NS_ERROR_TYPE_ERR;
}

::: dom/workers/ServiceWorkerWindowClient.cpp:456
(Diff revision 1)
> +    loadInfo->SetReferrerPolicy(doc->GetReferrerPolicy());
> +    loadInfo->SetLoadType(nsIDocShellLoadInfo::loadStopContentAndReplace);
> +    loadInfo->SetSourceDocShell(docShell);
> +    rv = docShell->LoadURI(url, loadInfo, nsIWebNavigation::LOAD_FLAGS_NONE, true);
> +
> +    if (NS_FAILED(rv)) {

NS_WARN_IF

Comment 5

[Andrea Marchesini [:baku]]

Comment on attachment 8761223 [details]
Bug 1218148 - Web Platform tests for WindowClient.navigate()

https://reviewboard.mozilla.org/r/58522/#review55638

::: testing/web-platform/meta/MANIFEST.json
(Diff revision 1)
>          "path": "dom/nodes/Node-isSameNode.html",
>          "url": "/dom/nodes/Node-isSameNode.html"
>        },
>        {
> -        "path": "dom/nodes/Node-isConnected.html",
> -        "url": "/dom/nodes/Node-isConnected.html"

Why this change?

::: testing/web-platform/tests/service-workers/service-worker/resources/client-navigate-worker.js:54
(Diff revision 1)
> +
> +    promise_test(function(t) {
> +      this.add_cleanup(function() { port.postMessage(pass(test, "")); });
> +      return self.clients.get(clientId)
> +                 .then(client => promise_rejects(t, new TypeError(), client.navigate("about:blank")))
> +                 .catch(unreached_rejection(t));

add a test where we reject because the URL is not valid.

Comment 6

[Andreas Farre [:farre]]

https://reviewboard.mozilla.org/r/58522/#review55638

> Why this change?

I ran the script for adding a test, which made this change. I guess it's a manual change, which the script has now fixed (it's only a move right?).

Comment 7

[Andreas Farre [:farre]]

Comment on attachment 8761222 [details]
Bug 1218148 - Implement WindowClient.navigate()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58520/diff/1-2/

Comment 8

[Andreas Farre [:farre]]

Comment on attachment 8761223 [details]
Bug 1218148 - Web Platform tests for WindowClient.navigate()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58522/diff/1-2/

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 8761222 [details]
Bug 1218148 - Implement WindowClient.navigate()

https://reviewboard.mozilla.org/r/58520/#review56040

::: dom/workers/ServiceWorkerWindowClient.cpp:278
(Diff revision 2)
> +  OnProgressChange(nsIWebProgress* aWebProgress, nsIRequest* aRequest,
> +                   int32_t aCurSelfProgress, int32_t aMaxSelfProgress,
> +                   int32_t aCurTotalProgress,
> +                   int32_t aMaxTotalProgress) override
> +  {
> +    MOZ_ASSERT(false, "Unexpected notification.");

MOZ_CRASH

::: dom/workers/ServiceWorkerWindowClient.cpp:366
(Diff revision 2)
> +    }
> +
> +    nsCOMPtr<nsIURI> baseUrl;
> +    nsCOMPtr<nsIURI> url;
> +    nsresult rv = ParseUrl(getter_AddRefs(baseUrl), getter_AddRefs(url));
> +    NS_ENSURE_SUCCESS(rv, rv);

remove this

::: dom/workers/ServiceWorkerWindowClient.cpp:384
(Diff revision 2)
> +      return RejectPromise(rv);
> +    }
> +
> +    nsCOMPtr<nsIDocShell> docShell = window->GetDocShell();
> +    nsCOMPtr<nsIWebProgress> webProgress = do_GetInterface(docShell);
> +    if (!webProgress) {

NS_WARN_IF

::: dom/workers/ServiceWorkerWindowClient.cpp:393
(Diff revision 2)
> +    nsCOMPtr<nsIWebProgressListener> listener =
> +      new WebProgressListener(mPromiseProxy, window->GetOuterWindow(), baseUrl);
> +
> +    rv = webProgress->AddProgressListener(
> +      listener, nsIWebProgress::NOTIFY_STATE_DOCUMENT);
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

This can fail. Probably it's better to a simple: NS_WARN_IF + FAILED + RejectPromise(rv);

::: dom/workers/ServiceWorkerWindowClient.cpp:398
(Diff revision 2)
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));
> +    return NS_OK;
> +  }
> +
> +private:
> +  nsresult RejectPromise(nsresult aRv)

nsresult
RejectPromise(nsresult aRv)

::: dom/workers/ServiceWorkerWindowClient.cpp:408
(Diff revision 2)
> +
> +    resolveRunnable->Dispatch();
> +    return NS_OK;
> +  }
> +
> +  nsresult ResolvePromise(UniquePtr<ServiceWorkerClientInfo>&& aClientInfo)

same indentation here.

::: dom/workers/ServiceWorkerWindowClient.cpp:421
(Diff revision 2)
> +    return NS_OK;
> +  }
> +
> +  nsresult
> +  ParseUrl(nsIURI** aBaseUrl, nsIURI** aUrl)
> +  {

MOZ_ASSERT(NS_IsOnMainThread());
MOZ_ASSERT(aBaseUrl)
MOZ_ASSERT(aUrl)

::: dom/workers/ServiceWorkerWindowClient.cpp:438
(Diff revision 2)
> +    return NS_OK;
> +  }
> +
> +  nsresult
> +  Navigate(nsIURI* aUrl, nsIPrincipal* aPrincipal, nsGlobalWindow** aWindow)
> +  {

MOZ_ASSERT(aWindow);

::: dom/workers/ServiceWorkerWindowClient.cpp:460
(Diff revision 2)
> +      return NS_ERROR_TYPE_ERR;
> +    }
> +
> +    nsCOMPtr<nsIDocShellLoadInfo> loadInfo;
> +    nsresult rv = docShell->CreateLoadInfo(getter_AddRefs(loadInfo));
> +

extra line?

Comment 10

[Andreas Farre [:farre]]

Comment on attachment 8761222 [details]
Bug 1218148 - Implement WindowClient.navigate()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58520/diff/2-3/

Comment 11

[Andreas Farre [:farre]]

Comment on attachment 8761223 [details]
Bug 1218148 - Web Platform tests for WindowClient.navigate()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58522/diff/2-3/

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Note, there is some additional spec discussion here:

https://github.com/slightlyoff/ServiceWorker/issues/918

Comment 13

[Andreas Farre [:farre]]

I'll fix that.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Actually, it seems like maybe you already do that?  It seems like the patch is using nsIWebNavigation::LOAD_FLAGS_NONE and not nsIWebNavigation::LOAD_FLAGS_REPLACE_HISTORY.

Comment 15

[Andreas Farre [:farre]]

Ah, I had it replacing in an intermediary version. Still, I'll make sure to await and see how the discussion the issue turns out.

Comment 16

[Andrea Marchesini [:baku]]

https://reviewboard.mozilla.org/r/58520/#review58374

Comment 17

[Andreas Farre [:farre]]

Comment on attachment 8761222 [details]
Bug 1218148 - Implement WindowClient.navigate()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58520/diff/3-4/

Comment 18

[Andreas Farre [:farre]]

Comment on attachment 8761223 [details]
Bug 1218148 - Web Platform tests for WindowClient.navigate()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58522/diff/3-4/

Comment 19

[Andreas Farre [:farre]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=df446e476717

Comment 20

[Carsten Book [:Tomcat]]

has problems to apply:

patching file dom/workers/ServiceWorkerWindowClient.cpp
Hunk #2 FAILED at 40
1 out of 4 hunks FAILED -- saving rejects to file dom/workers/ServiceWorkerWindowClient.cpp.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue

Comment 21

[Andreas Farre [:farre]]

Comment on attachment 8761222 [details]
Bug 1218148 - Implement WindowClient.navigate()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58520/diff/4-5/

Comment 22

[Andreas Farre [:farre]]

Comment on attachment 8761223 [details]
Bug 1218148 - Web Platform tests for WindowClient.navigate()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58522/diff/4-5/

Comment 23

[Andreas Farre [:farre]]

Rebased and pushed to mozreview. Now the patch should apply cleanly.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa2088d0085a
Implement WindowClient.navigate() r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d290ccf62a7
Web Platform tests for WindowClient.navigate() r=baku

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aa2088d0085a
https://hg.mozilla.org/mozilla-central/rev/7d290ccf62a7

Comment 26

[Eric Shepherd [:sheppy]]

(In reply to Andrew Overholt [:overholt] from comment #1)
> Looks like we may already have docs (if not an implementation), Jean-Yves:
> https://developer.mozilla.org/en-US/docs/Web/API/WindowClient/navigate

Yes, but we need to be sure it's on Firefox X for developers. Which it now is: https://developer.mozilla.org/en-US/Firefox/Releases/50#Service_Workers