Closed Bug 1030707 Opened 6 years ago Closed 6 years ago

Change usages of nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI and derivatives for bug 951991 - batch 8 - The Emancipation of the Workers

Categories

(Core :: DOM: Workers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: bobowen, Assigned: bobowen)

References

Details

Attachments

(8 files, 4 obsolete files)

1.95 KB, patch
bholley
: review+
Details | Diff | Splinter Review
1.46 KB, patch
bholley
: review+
Details | Diff | Splinter Review
2.06 KB, patch
bholley
: review+
Details | Diff | Splinter Review
1.32 KB, patch
bholley
: review+
Details | Diff | Splinter Review
1.33 KB, patch
bholley
: review+
Details | Diff | Splinter Review
1.42 KB, patch
bholley
: review+
Details | Diff | Splinter Review
5.93 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
7.02 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
Another batch of patches moving nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI etc.
See bug 951991 for details.
Component: DOM → DOM: Workers
Assignee: nobody → bobowencode
Looking fairly green on my try push, so I'll start uploading these:
https://tbpl.mozilla.org/?tree=Try&rev=790dc14d8cc7

Given the clear after the structured clone read, doesn't look like we'd need legacy error reporting there.
The mBackingStore->Put doesn't directly use the context and I think it eventually uses the one set up in the CallbackObject::CallSetup.
Attachment #8448009 - Flags: review?(bobbyholley)
Virtually the same as Part 1.

I meant to mention this before ...
The main difference with my replacement code (apart from just using the SafeJSContext) is that if something goes wrong with the AutoJSAPI Init, then I am throwing an error and returning.
Whereas before, if the JSContext set up failed, it asserted.
Attachment #8448021 - Flags: review?(bobbyholley)
Given the JS_ReportPendingException the legacy reporting looks like the safest option.
Attachment #8448024 - Flags: review?(bobbyholley)
Pretty much the same as Part 3.
Attachment #8448185 - Flags: review?(bobbyholley)
Attachment #8448009 - Flags: review?(bobbyholley) → review+
Attachment #8448021 - Flags: review?(bobbyholley) → review+
Attachment #8448024 - Flags: review?(bobbyholley) → review+
Attachment #8448185 - Flags: review?(bobbyholley) → review+
Had to do this one before the last one in RuntimeService, so I could rip out the nsIScriptContext pararmeter.

Apart from that, this is pretty similar to the last two.
Attachment #8448199 - Flags: review?(bobbyholley)
Now the scx parameter has gone this is another along the same vein.
Attachment #8448202 - Flags: review?(bobbyholley)
Given the xpc::Throw I've not looked into this too deeply and just gone for legacy reporting.
Attachment #8448208 - Flags: review?(bobbyholley)
Assuming that the current code is correct, where we are using the passed in aCx for the rooted dictionaries, it seems a bit odd that we are pushing the contexts where we are.

So, given the contexts are only being used to report errors when something has gone wrong, it seems to make sense to move them into those blocks.
Attachment #8448223 - Flags: review?(bobbyholley)
Comment on attachment 8448199 [details] [diff] [review]
Part 5: Replace AutoPushJSContext in SynchronizeAndResumeRunnable::Run v1

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

r=bholley with that.

::: dom/workers/WorkerPrivate.h
@@ +341,3 @@
>  
>    bool
>    Suspend(JSContext* aCx, nsPIDOMWindow* aWindow);

Can you add a comment above these 3 methods stating that we can assume the existence of a Window because this stuff only applies to globals going in and out of the bfcache? In general, Worker stuff needs to work with non-Window globals as well, so this threw me for a loop.
Attachment #8448199 - Flags: review?(bobbyholley) → review+
Attachment #8448202 - Flags: review?(bobbyholley) → review+
Attachment #8448208 - Flags: review?(bobbyholley) → review+
Comment on attachment 8448223 [details] [diff] [review]
Part 8: Replace AutoCxPusher in WorkerPrivateParent<Derived>::DispatchMessageEventToMessagePort v1

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

Instead of doing this, can we just make a version of Throw() that takes a DOM Window? That'll be less AutoJSAPI-ing and make it easier to eventually transition away from JSContexts entirely here.
Attachment #8448223 - Flags: review?(bobbyholley)
(In reply to Bobby Holley (:bholley) (PTO 6/25-6/29) from comment #10)
> Comment on attachment 8448223 [details] [diff] [review]
> Part 8: Replace AutoCxPusher in
> WorkerPrivateParent<Derived>::DispatchMessageEventToMessagePort v1
> 
> Review of attachment 8448223 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Instead of doing this, can we just make a version of Throw() that takes a
> DOM Window? That'll be less AutoJSAPI-ing and make it easier to eventually
> transition away from JSContexts entirely here.

Makes sense, I think it might apply in some other places already as well.
Anyway, time for sleep.
r=bholley - from comment 9.

Added requested comment.
Attachment #8448199 - Attachment is obsolete: true
Attachment #8448746 - Flags: review+
Added a ThrowAndReport method that takes a window, as the exceptions were reported straight afterwards.

It's a bit odd that the Throw always returns false.
Attachment #8448223 - Attachment is obsolete: true
Attachment #8448857 - Flags: review?(bobbyholley)
Whoops, forgot the try push:
https://tbpl.mozilla.org/?tree=Try&rev=2a18ae261bfc
Comment on attachment 8448857 [details] [diff] [review]
Part 8: Replace AutoCxPusher in WorkerPrivateParent<Derived>::DispatchMessageEventToMessagePort v2

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

::: dom/bindings/Exceptions.cpp
@@ +132,5 @@
> +  if (NS_WARN_IF(!jsapi.InitWithLegacyErrorReporting(aWindow))) {
> +    return false;
> +  }
> +
> +  // This version of Throw always returns false for some reason.

so that people can just do "return Throw(..)". I don't necessarily agree with this design pattern, but it's common enough that I don't think we need this comment.

@@ +134,5 @@
> +  }
> +
> +  // This version of Throw always returns false for some reason.
> +  Throw(jsapi.cx(), aRv, aMessage);
> +  return JS_ReportPendingException(jsapi.cx());

I don't think the return value of JS_ReportPendingException is all that useful. I think this function should return void and the last line should explicitly cast the result of JS_ReportPendingException to void.
Attachment #8448857 - Flags: review?(bobbyholley) → review+
r=bholley - from comment 15

(In reply to Bobby Holley (:bholley) from comment #15)

> > +  // This version of Throw always returns false for some reason.
> 
> so that people can just do "return Throw(..)". I don't necessarily agree
> with this design pattern, but it's common enough that I don't think we need
> this comment.

Ah, I see, comment removed.
I've not seen that before, probably because most of my experience of this style of coding has been with "proper" exception handling.

I suppose it saves a line ... but if I were using it, I'd call the function ThrowAndReturnFalse (or something), to make it explicitly clear what was going on.
Particularly when the other Throws (albeit with different names) don't use this pattern.
 
> > +  return JS_ReportPendingException(jsapi.cx());
> 
> I don't think the return value of JS_ReportPendingException is all that
> useful. I think this function should return void and the last line should
> explicitly cast the result of JS_ReportPendingException to void.

OK, I'm guessing that this is a last ditch attempt to report the error and if it failed there's not much we could do anyway.
Attachment #8448857 - Attachment is obsolete: true
Attachment #8449276 - Flags: review+
Attachment #8449276 - Attachment description: Replace AutoCxPusher in WorkerPrivateParent<Derived>::DispatchMessageEventToMessagePort v3 → Part 8: Replace AutoCxPusher in WorkerPrivateParent<Derived>::DispatchMessageEventToMessagePort v3
r=bholley - from comment 15 with points addressed in comment 16.

I noticed, in one of your patches for Bug 1032317, that C-style casting seems to be the norm when deliberately ignoring a return value.

Sorry for the spam.
Attachment #8449276 - Attachment is obsolete: true
Attachment #8449316 - Flags: review+
Fairly full try push before reviews:
https://tbpl.mozilla.org/?tree=Try&rev=790dc14d8cc7

Smaller try push after first changes to Part 8:
https://tbpl.mozilla.org/?tree=Try&rev=2a18ae261bfc

Compiled and tested locally after the final changes, but I think they're small enough to not warrant another try push ... famous last words I'm sure. :-)

Landing in patch number order please.
Thanks.
Status: NEW → ASSIGNED
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.