Closed Bug 1259290 Opened 5 years ago Closed 5 years ago

Remove a bunch of unnecessary JS_ReportError calls in dom/workers


(Core :: DOM: Workers, defect)

Not set



Tracking Status
firefox48 --- fixed


(Reporter: khuey, Assigned: khuey)




(5 files)

More exception handling cleanup.
These JS_ReportError callsites are either already throwing on the ErrorResult, or can be trivially made to do so.
Attachment #8734196 - Flags: review?(bzbarsky)
There's no need for this since we dropped the cx from ModifyBusyCountFromWorker.
Attachment #8734197 - Flags: review?(bzbarsky)
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 ( 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)
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 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.

Attachment #8734196 - Flags: review?(bzbarsky) → review+
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 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...

Attachment #8734199 - Flags: review?(bzbarsky) → review+
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+
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)
Comment on attachment 8735000 [details] [diff] [review]
Part 5: Remove aCx from WorkerFeature::Notify


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+
Comment on attachment 8734199 [details] [diff] [review]
Part 3: RegisterWorker

Got r=jandem on irc.
Attachment #8734199 - Flags: review?(jorendorff) → review+
You need to log in before you can comment on or make changes to this bug.