Closed Bug 1448522 Opened 6 years ago Closed 6 years ago

Handle errors in transferOwnership correctly

Categories

(Core :: JavaScript Engine, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

Details

Attachments

(1 file, 1 obsolete file)

Noticed in bug 1441141 comment 11, there are two places that throw uncatchable exceptions.
Attachment #8961992 - Flags: review?(jorendorff)
Priority: -- → P2
Comment on attachment 8961992 [details] [diff] [review]
Handle errors in transferOwnership correctly

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

::: js/src/tests/non262/extensions/clone-transferables.js
@@ +98,2 @@
>                  get foo() {
> +                    deserialize(serialize(b1, [b1], { scope }), { scope });

Pre-existing, but is the `deserialize` necessary here? The intended effect is to detach the buffer, and `serialize` does that... right?

@@ +103,1 @@
>              // The throw is not yet implemented, bug 919259.

I have a strong feeling I'm missing something, but: doesn't this patch implement the throw?

If so, the try/catch should change so that we test for the exception and fail if it isn't thrown.

Also, pre-existing English nit: Can we please not noun the word "throw"?

@@ +110,5 @@
> +            const b2 = new ArrayBuffer(size);
> +            mutator = {
> +                get foo() {
> +                    deserialize(serialize(b2, [b2], { scope }), { scope });
> +                    detachArrayBuffer(b2);

Did you mean to do both `serialize` and `detachArrayBuffer` here? It seems like just the latter is what you want.
Attachment #8961992 - Flags: review?(jorendorff) → review+
(In reply to Jason Orendorff [:jorendorff] from comment #2)
> Also, pre-existing English nit: Can we please not noun the word "throw"?

Obligatory "That seems like a reasonable ask" to preempt anyone else saying it. I don't think I need to name names.
(In reply to Jason Orendorff [:jorendorff] from comment #2)
> Comment on attachment 8961992 [details] [diff] [review]
> Handle errors in transferOwnership correctly
> 
> Review of attachment 8961992 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/tests/non262/extensions/clone-transferables.js
> @@ +98,2 @@
> >                  get foo() {
> > +                    deserialize(serialize(b1, [b1], { scope }), { scope });
> 
> Pre-existing, but is the `deserialize` necessary here? The intended effect
> is to detach the buffer, and `serialize` does that... right?
> 
> @@ +103,1 @@
> >              // The throw is not yet implemented, bug 919259.
> 
> I have a strong feeling I'm missing something, but: doesn't this patch
> implement the throw?

What you're missing is something related to my level of competence, so let's not go into that here.

> If so, the try/catch should change so that we test for the exception and
> fail if it isn't thrown.

Yes, indeed.

> Also, pre-existing English nit: Can we please not noun the word "throw"?

Sure, I can do that replace for you before I do the final land.

Argh, you already did that one better, didn't you?

> @@ +110,5 @@
> > +            const b2 = new ArrayBuffer(size);
> > +            mutator = {
> > +                get foo() {
> > +                    deserialize(serialize(b2, [b2], { scope }), { scope });
> > +                    detachArrayBuffer(b2);
> 
> Did you mean to do both `serialize` and `detachArrayBuffer` here? It seems
> like just the latter is what you want.

Good point.
I spent some quality time with this bug and refreshed my memory.

My patch did *not* fix bug 919259; it only fixed up oom handling. That overlaps a little with the detachment handling, since now the test case *can* throw catchable exceptions, but probably won't.

On the other hand, it turns out that bug 919259 is trivially fixable now, so I may as well sneak it in, since it results in a cleaner patch than fixing the oom problem alone. So I did that, but it's different, so re-requesting review.
Attachment #8968670 - Flags: review?(jorendorff)
Attachment #8961992 - Attachment is obsolete: true
Attachment #8968670 - Flags: review?(jorendorff) → review+
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee36b000e3ad
Handle errors in transferOwnership correctly, r=jorendorff
https://hg.mozilla.org/mozilla-central/rev/ee36b000e3ad
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.