Closed
Bug 1037725
Opened 10 years ago
Closed 8 years ago
Warn in console about worker limit being exceeded
Categories
(Core :: DOM: Workers, defect)
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.
Whiteboard: [tw-dom]
Mentor: khuey
Version: 24 Branch → Trunk
Updated•8 years ago
|
Whiteboard: [tw-dom] → [tw-dom] btpp-active
Assignee | ||
Comment 2•8 years ago
|
||
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 6•8 years ago
|
||
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.
Assignee | ||
Comment 9•8 years ago
|
||
- 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
Assignee | ||
Comment 10•8 years ago
|
||
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
Assignee | ||
Comment 11•8 years ago
|
||
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
Assignee | ||
Comment 12•8 years ago
|
||
- Rebase the patch.
Attachment #8732046 -
Attachment is obsolete: true
Attachment #8738607 -
Flags: review?(khuey)
Assignee | ||
Comment 13•8 years ago
|
||
Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=027dc8879bca70b82bb06f293c86089236309c66
Attachment #8736671 -
Attachment is obsolete: true
Attachment #8738609 -
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-
Assignee | ||
Comment 16•8 years ago
|
||
Thanks for pointing out all the errors. I will address them in next patch.
Assignee | ||
Comment 17•8 years ago
|
||
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+
Assignee | ||
Comment 19•8 years ago
|
||
Address comment 18 and carry on r+. Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=184c6ddabf149a74061365837bc2dbc89901ea28
Attachment #8739995 -
Attachment is obsolete: true
Attachment #8740309 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 20•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca9577b9e37a
Keywords: checkin-needed
Comment 21•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ca9577b9e37a
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•