1312788 - Add console warning and telemetry if service workers are used in the file content process.

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Bob Owen (:bobowen)]

In bug 1147911 we're changing things to load file:// URL web pages in their own separate content process.

Normally service workers aren't allowed on file:// URLs.
However file:// URLs are allowed to frame normal web content, so there is a concern that this web content could use service workers and run into the e10s-multi issues service workers currently have.

We think this is an edge case on which we shouldn't block, but we ought to add in a console warning and telemetry, if it does occur.

Comment 1

[David Parks [:handyman]]

Attachment: WIP: ServiceWorker test

Hacked up from the source in this GitHub repo:

https://github.com/jakearchibald/simple-serviceworker-tutorial

This archive contains a few pages that can be used to test this patch.  Specifically, it contains some pages (index.html, index2.html index3.html) that reference other pages (index-inner.html, ..., index-inner3.html) in iframes.  The inner pages are loaded via http:// and contain a ServiceWorker.

The pages can be accessed via http:// (by installing node.js and running npm install && npm start in the project folder, then going to e.g. http://localhost:8080/index3.html).  We do not want to log a message or telemetry in this case.

The pages can also be accessed by file links (Ctrl-O/Cmd-O).  The npm commands MUST STILL BE ISSUED FIRST since the inner frames are loaded from the node server.  In this case, the messages requested by this bug should be logged.

Comment 2

[David Parks [:handyman]]

Attachment: Log + Add Telemetry for cases of ServiceWorkers being used in embedded file:// URIs

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

Comment 3

[David Parks [:handyman]]

Comment on attachment 8820566 [details] [diff] [review]
Log + Add Telemetry for cases of ServiceWorkers being used in embedded file:// URIs

Rerun of tests (prior had win x64 try machine issues):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a39ff4659f55c2b2a62d18b2b8b5caa6b4913c8

Comment 4

[Bob Owen (:bobowen)]

Comment on attachment 8820566 [details] [diff] [review]
Log + Add Telemetry for cases of ServiceWorkers being used in embedded file:// URIs

Review of attachment 8820566 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry I wasn't clear in the description, we want to log to the Web Console (or possibly Browser Console if that's not possible), so that a web developer might see the warning.

Looks like there might be some helpers already in the service worker stuff to do this.

Also, it should really be a Web Workers peer to review this.

::: dom/workers/ServiceWorkerManager.cpp
@@ +624,5 @@
>    AssertIsOnMainThread();
>    Telemetry::Accumulate(Telemetry::SERVICE_WORKER_REGISTRATIONS, 1);
>  
> +  MOZ_ASSERT(mActor);
> +  mActor->SendCheckIfInFileRemoteContent();

I don't think we need any new IPDL here, we can just do something like:

  ContentChild* contentChild = ContentChild::GetSingleton();
  if (contentChild->GetRemoteType().EqualsLiteral(FILE_REMOTE_TYPE)) {
    // log and accumulate
  }

Comment 5

[Bob Owen (:bobowen)]

Re comment 4.

What's the best way for David to log to the Web Console from service worker code?

Also, do you think we need localisation for this or is this a small enough edge case to not warrant it?

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

You can use either ServiceWorkerManager::ReportToAllClients():

https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.h#210

Or LocalizeAndReportToAllClients():

https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.h#236

However, I agree its not worth localizing this.  Its a corner case and we are also going to be removing this warning in the next few releases.

Note, both of these calls require you to be on the main thread.

Comment 7

[David Parks [:handyman]]

Attachment: Log + Add Telemetry for cases of ServiceWorkers being used in embedded file:// URIs

Addresses Bob's points in comment 4.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8824579 [details] [diff] [review]
Log + Add Telemetry for cases of ServiceWorkers being used in embedded file:// URIs

Review of attachment 8824579 [details] [diff] [review]:
-----------------------------------------------------------------

A couple nits, but otherwise looks good to me.  r=me with comments addressed.

::: dom/workers/ServiceWorkerManager.cpp
@@ +629,5 @@
> +  ContentChild* contentChild = ContentChild::GetSingleton();
> +  if (contentChild &&
> +      contentChild->GetRemoteType().EqualsLiteral(FILE_REMOTE_TYPE)) {
> +    nsString message;
> +    message.AppendLiteral("ServiceWorker registered by document embedded in a file:/// URI.");

nit: Can you add some warning text?  Something like "This may result in unexpected behavior."

nit: Also, please just use NS_LITERAL_STRING("my message").  You can either do:

  nsString message(NS_LITERAL_STRING("foo"));

Or:

  ReportToAllClients(cleanedScope,
                     NS_LITERAL_STRING("foo"),
                     /* other args */);

Comment 9

[David Parks [:handyman]]

Attachment: Log + Add Telemetry for ServiceWorkers used in file:// URIs (r=bkelly)

This takes care of bkelly's nits.

Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=512e458bd8c4778484190657e4e59f45a5f00082

Comment 10

[Ryan VanderMeulen [:RyanVM]]

All new Telemetry probes need to go through data review per https://wiki.mozilla.org/Firefox/Data_Collection#Requesting_Approval

Comment 11

[David Parks [:handyman]]

Comment on attachment 8825588 [details] [diff] [review]
Log + Add Telemetry for ServiceWorkers used in file:// URIs (r=bkelly)

Understood.  This needs telemetry feedback -- its a pretty basic probe.

Comment 12

[Benjamin Smedberg]

Comment on attachment 8825588 [details] [diff] [review]
Log + Add Telemetry for ServiceWorkers used in file:// URIs (r=bkelly)

data-r=me

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6257972e6a6
Log to browser console and telemetry if the parent content to a ServiceWorker is a file. r=bkelly

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e6257972e6a6

Comment 15

[Simona Badau, Desktop QA]

Verified as fixed using the latest Nightly 55.0a1 (Build ID: 20170403030207) on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12.
The message displayed in the console is: ServiceWorker registered by document embedded in a file:/// URI.  This may result in unexpected behavior.
The parameter displayed in the about:telemetry is: FILE_EMBEDDED_SERVICEWORKERS.