Closed Bug 1037725 Opened 5 years ago Closed 4 years ago

Warn in console about worker limit being exceeded

Categories

(Core :: DOM: Workers, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: azakai, Assigned: jdai, Mentored)

Details

(Whiteboard: [tw-dom] btpp-active)

Attachments

(1 file, 6 obsolete files)

After 20 workers, we no longer allow more workers to run, in order to prevent DOS. What happens is the worker is created (new Worker() succeeds) but never starts, ignores postMessages, etc. This is confusing for users, I think. A warning in the console would be useful.

Testcase is in bug 1021274.
I would like to try on this bug.
Assignee: nobody → jdai
Mentor: khuey
Version: 24 Branch → Trunk
Whiteboard: [tw-dom] → [tw-dom] btpp-active
When worker spawn over limit, it should always report warning to console.

try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4cfe760cbd9
Attachment #8730496 - Flags: review?(khuey)
Comment on attachment 8730496 [details] [diff] [review]
Bug 1037725 - Add warning message in the console when worker spawn over limit.

Axel, is it ok to change a message like this? Or do we have to create a new one?
Attachment #8730496 - Flags: feedback?(l10n)
Comment on attachment 8730496 [details] [diff] [review]
Bug 1037725 - Add warning message in the console when worker spawn over limit.

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

::: dom/workers/RuntimeService.cpp
@@ +1422,5 @@
>               !exemptFromPerDomainMax;
>  
>      if (queued) {
>        domainInfo->mQueuedWorkers.AppendElement(aWorkerPrivate);
> +      AssertIsOnMainThread();

There's nothing that guarantees this will be on the main thread.  For Service and Shared Workers we must be on the main thread, because Workers cannot spawn Service or Shared Workers.  But regular Workers can spawn other Workers, so we could be on a Worker thread here.

You'll need to proxy the nsContentUtils::ReportToConsole call to the main thread if we're not on it already.
Attachment #8730496 - Flags: review?(khuey) → review-
(You could easily write a test for that, by having a Worker recursively load itself as a Worker.)
Comment on attachment 8730496 [details] [diff] [review]
Bug 1037725 - Add warning message in the console when worker spawn over limit.

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

While I hate retranslating strings for one word, the localization comment asks to not translate "ServiceWorker", which means locales should have that word. 
https://transvision.mozfr.org/string/?entity=dom/chrome/dom/dom.properties:HittingMaxWorkersPerDomain&repo=aurora

If you want localized builds to say "Worker" too, the only safe way is to change the string ID.

Not sure why the change from ServiceWorker to Worker for one string though
https://transvision.mozfr.org/?recherche=ServiceWorker&repo=central&sourcelocale=en-US&locale=en-US&search_type=strings&case_sensitive=case_sensitive
Attachment #8730496 - Flags: feedback?(l10n) → feedback-
(In reply to Francesco Lodolo [:flod] from comment #6)
> Comment on attachment 8730496 [details] [diff] [review]
> Bug 1037725 - Add warning message in the console when worker spawn over
> limit.
> 
> Review of attachment 8730496 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> While I hate retranslating strings for one word, the localization comment
> asks to not translate "ServiceWorker", which means locales should have that
> word. 
> https://transvision.mozfr.org/string/?entity=dom/chrome/dom/dom.properties:
> HittingMaxWorkersPerDomain&repo=aurora

Right.

> If you want localized builds to say "Worker" too, the only safe way is to
> change the string ID.

Ok.

> Not sure why the change from ServiceWorker to Worker for one string though
> https://transvision.mozfr.org/
> ?recherche=ServiceWorker&repo=central&sourcelocale=en-US&locale=en-
> US&search_type=strings&case_sensitive=case_sensitive

Well we're changing how the code works, so ...
Comment on attachment 8730496 [details] [diff] [review]
Bug 1037725 - Add warning message in the console when worker spawn over limit.

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

::: dom/locales/en-US/chrome/dom/dom.properties
@@ +161,5 @@
>  IndexedDBTransactionAbortNavigation=An IndexedDB transaction that was not yet complete has been aborted due to page navigation.
>  # LOCALIZATION NOTE: Do not translate Will-change, %1$S,%2$S are numbers.
>  IgnoringWillChangeOverBudgetWarning=Will-change memory consumption is too high. Budget limit is the document surface area multiplied by %1$S (%2$S px). Occurrences of will-change over the budget will be ignored.
> +# LOCALIZATION NOTE: Do not translate "Worker".
> +HittingMaxWorkersPerDomain=A Worker could not be started immediately because other documents in the same origin are already using the maximum number of workers. The Worker is now queued and will be started after some of the other workers have completed.

per :flod above, we'll need to change this to HittingMaxWorkersPerDomain2= or something similar.
- Add ReportErrorOnConsole to proxy the nsContentUtils::ReportToConsole call to the main thread.
- Add HittingMaxWorkersPerDomain2 message.

TODO: add a worker test case
Attachment #8730496 - Attachment is obsolete: true
It hits dom/workers/test/test_bug1241485.html and the error message is "Too many workers created!". I need to tweak my testcase because I use fibonacci sequence generate 55 subworkers.

Try result:https://treeherder.mozilla.org/#/jobs?repo=try&revision=28631c92a5a1
The problem came from my code changed[1]. It will generate more than max domain workers. I put another try[2] to test it.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.cpp#1547
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=801dfca36c27
- Rebase the patch.
Attachment #8732046 - Attachment is obsolete: true
Attachment #8738607 - Flags: review?(khuey)
Comment on attachment 8738607 [details] [diff] [review]
Part 1: Bug 1037725 - Add warning message in the console when worker spawn over limit. v3

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

Getting close.  Some comments.

::: dom/locales/en-US/chrome/dom/dom.properties
@@ +161,5 @@
>  IndexedDBTransactionAbortNavigation=An IndexedDB transaction that was not yet complete has been aborted due to page navigation.
>  # LOCALIZATION NOTE: Do not translate Will-change, %1$S,%2$S are numbers.
>  IgnoringWillChangeOverBudgetWarning=Will-change memory consumption is too high. Budget limit is the document surface area multiplied by %1$S (%2$S px). Occurrences of will-change over the budget will be ignored.
>  # LOCALIZATION NOTE: Do not translate "ServiceWorker".
>  HittingMaxWorkersPerDomain=A ServiceWorker could not be started immediately because other documents in the same origin are already using the maximum number of workers. The ServiceWorker is now queued and will be started after some of the other workers have completed.

We should get rid of the original message entirely.

::: dom/workers/RuntimeService.cpp
@@ +1456,5 @@
> +      if (parent) {
> +        // Worker spawn subworkers on a Worker thread which gets queued due to
> +        // hitting max workers per domain limit so let's log a warning.
> +        parent->AssertIsOnWorkerThread();
> +        parent->ReportErrorOnConsole("HittingMaxWorkersPerDomain2");

And these two branches should be consolidated.  Perhaps you should make ReportErrorToConsole a static method on WorkerPrivate, and then you can call it unconditionally here without caring which thread it's on.

::: dom/workers/WorkerPrivate.cpp
@@ +937,5 @@
>      return aWorkerPrivate->ThawInternal();
>    }
>  };
>  
> +class ReportErrorOnConsoleRunnable final : public WorkerRunnable

ReportErrorToConsole, here and everywhere else, please.

@@ +945,5 @@
> +public:
> +  // aWorkerPrivate is the worker thread we're on (or the main thread, if null)
> +  static void
> +  ReportErrorOnConsole(WorkerPrivate* aWorkerPrivate,
> +                       const nsCString& aMessage)

just make this take a char*, so you don't need the aMessage.get() later?

And perhaps shorten this to just "Report".

@@ +966,5 @@
> +    nsContentUtils::GetLocalizedString(nsContentUtils::eDOM_PROPERTIES,
> +                                       aMessage.get(), message);
> +
> +    // Log an error to the error console.
> +    LogErrorToConsole(message, nsString(), nsString(), 0, 0, 0, 0);

LogErrorToConsole is a bit different ... can we just use nsContentUtils::ReportToConsole here?  I'd rather not change the behavior if we don't have to.

@@ +995,5 @@
> +      if (aWorkerPrivate->IsFrozen() || aWorkerPrivate->IsSuspended()) {
> +        MOZ_ASSERT(!IsDebuggerRunnable());
> +        aWorkerPrivate->QueueRunnable(this);
> +        return true;
> +      }

We only need to queue runnables that could execute script or otherwise change state.  This just reports a message to the console, so it doesn't matter.  You can rewrite this all as

MOZ_ASSERT_IF(!parent, NS_IsMainThread());

@@ +5915,5 @@
>  }
>  
> +void
> +WorkerPrivate::ReportErrorOnConsole(const char* aMessage)
> +{

And then over here you can do something like:

static void
WorkerPrivate::ReportErrorToConsole(const char* aMessage) {
  WorkerPrivate* wp = nullptr;
  if (!NS_IsMainThread()) {
    wp = GetCurrentThreadWorkerPrivate();
  }

  ReportErrorToConsoleRunnable::Report(wp, message);
}

@@ +5921,5 @@
> +
> +  nsCString message;
> +  if (message.IsEmpty()) {
> +    message = nsCString(aMessage);
> +  }

message is always empty here, you just created it.
Attachment #8738607 - Flags: review?(khuey) → review-
Comment on attachment 8738609 [details] [diff] [review]
Part 2: Bug 1037725 - mochitest testcase. v2

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

I actually don't think you can write an automated test for this.  This test here doesn't actually check that the right messages are posted to the console.  Best to test this manually, I think.  And we have some functions in SimpleTest to do that but none of them work asynchronously.
Attachment #8738609 - Flags: review?(khuey) → review-
Thanks for pointing out all the errors. I will address them in next patch.
Address comment 14 and 15. I've manually verified that the warning message shows up in the console.

Try result:https://treeherder.mozilla.org/#/jobs?repo=try&revision=f186cfcaa525

Hi Kyle,

Could you help to review this patch? Thank you.
Attachment #8738607 - Attachment is obsolete: true
Attachment #8738609 - Attachment is obsolete: true
Attachment #8739995 - Flags: review?(khuey)
Comment on attachment 8739995 [details] [diff] [review]
Bug 1037725 - Add warning message in the console when worker spawn over limit. v4

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

::: dom/workers/WorkerPrivate.cpp
@@ +945,5 @@
> +public:
> +  // aWorkerPrivate is the worker thread we're on (or the main thread, if null)
> +  static void
> +  Report(WorkerPrivate* aWorkerPrivate,
> +                       const char* aMessage)

nit: aMessage on the previous line.
Attachment #8739995 - Flags: review?(khuey) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ca9577b9e37a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.