Closed
Bug 1259290
Opened 8 years ago
Closed 8 years ago
Remove a bunch of unnecessary JS_ReportError calls in dom/workers
Categories
(Core :: DOM: Workers, defect)
Core
DOM: Workers
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: khuey, Assigned: khuey)
References
Details
Attachments
(5 files)
5.07 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
3.57 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
11.31 KB,
patch
|
bzbarsky
:
review+
khuey
:
review+
|
Details | Diff | Splinter Review |
893 bytes,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
21.28 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
More exception handling cleanup.
Assignee | ||
Comment 1•8 years ago
|
||
These JS_ReportError callsites are either already throwing on the ErrorResult, or can be trivially made to do so.
Attachment #8734196 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 2•8 years ago
|
||
There's no need for this since we dropped the cx from ModifyBusyCountFromWorker.
Attachment #8734197 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 3•8 years ago
|
||
This is slightly complex. Both RegisterWorker and UnregisterWorker are called on the parent thread. RegisterWorker is called during construction, so throwing an exception is sensible, but UnregisterWorker is not, so throwing is nonsense. UnregisterWorker only needs a cx to pass to ScheduleWorker, to squelch any exceptions that ScheduleWorker raises, and to pass to itself. ScheduleWorker is called from UnregisterWorker and RegisterWorker. When called from RegisterWorker, RegisterWorker's caller explicitly throws an exception (http://hg.mozilla.org/mozilla-central/annotate/6202ade0e6d6/dom/workers/WorkerPrivate.cpp#l4088) if RegisterWorker fails in any way. And as mentioned before, UnregisterWorker squelches the exception. So we don't need to report anything more than a false return value from ScheduleWorker. ScheduleWorker also needs the cx to get the parent runtime, which can be fixed with a simple JSAPI modification. After that, ScheduleWorker only needs a cx to call UnregisterWorker, and UnregisterWorker only needs a cx to call ScheduleWorker and itself, so we can rip the cx out of both. As mentioned before, RegisterWorker's caller will throw on the ErrorResult if it fails, and the only non-exception handling needs of a cx are to call ScheduleWorker and UnregisterWorker, so we can rip out all of the exception handling and the cx from RegisterWorker too.
Attachment #8734199 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 4•8 years ago
|
||
Worker threads all shut down at xpcom-shutdown-threads (and we block shutdown to do so), so NS_DispatchToMainThread can never fail.
Attachment #8734200 -
Flags: review?(bzbarsky)
Comment 5•8 years ago
|
||
Comment on attachment 8734196 [details] [diff] [review] Part 1: The trivial cases > if (!runtimeService) { >- JS_ReportError(aCx, "Failed to create runtime service!"); In fairness, the JS_ReportError is more informative. Want to make this a ThrowDOMException with some actual DOMException error code and the informative string? >@@ -5839,17 +5838,18 @@ WorkerPrivate::SetTimeout(JSContext* aCx, Similar here. Add to that that NS_ERROR_FAILURE should probably never show up in "normal" script execution, since there's no spec that defines it. Of course per spec I don't think we're supposed to throw here either... > if (!sandbox) { >- JS_ReportError(aCx, "Can't create sandbox!"); >+ aRv.Throw(NS_ERROR_FAILURE); Shouldn't this one be aRv.NoteJSContextException(aCx)? Same for the other callsites in this function. r=me
Attachment #8734196 -
Flags: review?(bzbarsky) → review+
Comment 6•8 years ago
|
||
Comment on attachment 8734197 [details] [diff] [review] Part 2: Remove aCx from Add|RemoveChildWorker Ah, the cleanup frontier advances. ;) r=me
Attachment #8734197 -
Flags: review?(bzbarsky) → review+
Comment 7•8 years ago
|
||
Comment on attachment 8734199 [details] [diff] [review] Part 3: RegisterWorker Again, we're losing some information in terms of error messages, but that seems ok; in practice we're not expecting these codepaths to actually be hit, right? >class TopLevelWorkerFinishedRunnable final : public nsRunnable > Run() override >@@ -435,17 +435,17 @@ private: The "cx" in this function is now unused and can go away, right? >+JS_GetParentRuntime(JSRuntime* rt) You should probably leave the cx version around, at least until you get review from one of the JS API owners. I can probably review adding the version that takes rt, but not removing an existing thing... r=me
Attachment #8734199 -
Flags: review?(bzbarsky) → review+
Comment 8•8 years ago
|
||
Comment on attachment 8734200 [details] [diff] [review] Part 4: ScriptLoader r=me. I believe this is the last WorkerFeature subclass which used the JSContext for anything that's not an assert. So we should be able to nix the argument, I think then we can remove it from the one callsite where we notify features too...
Attachment #8734200 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 9•8 years ago
|
||
Comment on attachment 8734199 [details] [diff] [review] Part 3: RegisterWorker jorendorff, can you sign off on the JSAPI change here?
Attachment #8734199 -
Flags: review?(jorendorff)
Assignee | ||
Comment 10•8 years ago
|
||
Attachment #8735000 -
Flags: review?(bzbarsky)
Comment 11•8 years ago
|
||
Comment on attachment 8735000 [details] [diff] [review] Part 5: Remove aCx from WorkerFeature::Notify r=me Now the only uses of aCx in WorkerPrivate::NotifyFeatures are the !JS_IsExceptionPending asserts, so I suspect we can nix it too. If we do that, it looks like NotifyInternal still uses aCx for a bizarre sort of "did we ever create a global" flag... but I bet we could nix that too in favor of asking that question directly if we wanted to. Maybe there's not that much point, since all three of the callers have a cx readily available (though we could change close() to not take one, I guess, if we really cared).
Attachment #8735000 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 12•8 years ago
|
||
Comment on attachment 8734199 [details] [diff] [review] Part 3: RegisterWorker Got r=jandem on irc.
Attachment #8734199 -
Flags: review?(jorendorff) → review+
Comment 13•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a25d3a1c24b7 https://hg.mozilla.org/integration/mozilla-inbound/rev/780e64f25e1a https://hg.mozilla.org/integration/mozilla-inbound/rev/d317ee09ff1a https://hg.mozilla.org/integration/mozilla-inbound/rev/a4dbc5475d87 https://hg.mozilla.org/integration/mozilla-inbound/rev/e979e75c94f6
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a25d3a1c24b7 https://hg.mozilla.org/mozilla-central/rev/780e64f25e1a https://hg.mozilla.org/mozilla-central/rev/d317ee09ff1a https://hg.mozilla.org/mozilla-central/rev/a4dbc5475d87 https://hg.mozilla.org/mozilla-central/rev/e979e75c94f6
Status: ASSIGNED → 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
•