Closed Bug 1312788 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Security: Process Sandboxing, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed
firefox55 --- verified

People

(Reporter: bobowen, Assigned: handyman)

References

Details

(Whiteboard: sbwc2, sblc3, sbmc2,[e10s-multi:M2])

Attachments

(2 files, 2 obsolete files)

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.
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.
Assignee: nobody → davidp99
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
Attachment #8820566 - Flags: review?(bobowencode)
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
  }
Attachment #8820566 - Flags: review?(bobowencode)
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?
Flags: needinfo?(bkelly)
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.
Flags: needinfo?(bkelly)
Addresses Bob's points in comment 4.
Attachment #8820566 - Attachment is obsolete: true
Attachment #8824579 - Flags: review?(bkelly)
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 */);
Attachment #8824579 - Flags: review?(bkelly) → review+
Attachment #8824579 - Attachment is obsolete: true
Keywords: checkin-needed
All new Telemetry probes need to go through data review per https://wiki.mozilla.org/Firefox/Data_Collection#Requesting_Approval
Keywords: checkin-needed
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.
Attachment #8825588 - Flags: feedback?(benjamin)
Comment on attachment 8825588 [details] [diff] [review]
Log + Add Telemetry for ServiceWorkers used in file:// URIs (r=bkelly)

data-r=me
Attachment #8825588 - Flags: feedback?(benjamin) → feedback+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e6257972e6a6
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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.
You need to log in before you can comment on or make changes to this bug.