742384 - In CDataFinalizer, .dispose() should return something useful

Status: RESOLVED FIXED

(Core :: js-ctypes, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

At the moment, .dispose() returns undefined. It should return the result of the call to the finalizer.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: Returning the result during dispose

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Attachment: Returning the value after dispose

Here is a new version. It simplifies the code a little and adds tests.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 613564 [details] [diff] [review]
Returning the value after dispose

Looks good, r=me.

>+  jsval result = JSVAL_VOID;
>+
>+
>+  int errnoStatus;

Style nit: please remove one of the two blank lines here.

>+  if (ConvertToJS(cx, resultType, NULL, p->rvalue, false, true, &result)) {
>+    CDataFinalizer::Cleanup(p, obj);
>+    JS_SET_RVAL(cx, vp, result);
>+    return true;
>+  }
>   CDataFinalizer::Cleanup(p, obj);

It seems best to call Cleanup first:

    CDataFinalizer::Cleanup(p, obj);
    if (!ConvertToJS(cx, resultType, NULL, p->rvalue, false, true, &result)) {
      return false;
    }
    JS_SET_RVAL(cx, vp, result);
    return true;

This is just defensive programming: after the finalizer is called, obj is supposed to be empty.

See, only two little comments! SpiderMonkey style can be learned! :)

Comment 4

[David Teller [:Yoric] - still alive but not very active]

(In reply to Jason Orendorff [:jorendorff] from comment #3)
> Comment on attachment 613564 [details] [diff] [review]
> Returning the value after dispose
> 
> Looks good, r=me.
> 
> >+  jsval result = JSVAL_VOID;
> >+
> >+
> >+  int errnoStatus;
> 
> Style nit: please remove one of the two blank lines here.
> 
> >+  if (ConvertToJS(cx, resultType, NULL, p->rvalue, false, true, &result)) {
> >+    CDataFinalizer::Cleanup(p, obj);
> >+    JS_SET_RVAL(cx, vp, result);
> >+    return true;
> >+  }
> >   CDataFinalizer::Cleanup(p, obj);
> 
> It seems best to call Cleanup first:
> 
>     CDataFinalizer::Cleanup(p, obj);
>     if (!ConvertToJS(cx, resultType, NULL, p->rvalue, false, true, &result))
> {
>       return false;
>     }
>     JS_SET_RVAL(cx, vp, result);
>     return true;
> 
> This is just defensive programming: after the finalizer is called, obj is
> supposed to be empty.

Actually, |Cleanup| cleans |p| and |p->rvalue|, which I need in the call to |ConvertToJS|. So, I suppose I could do that, but I would first need to copy |p->rvalue| and then clean that copy. Do you think it is useful?

> See, only two little comments! SpiderMonkey style can be learned! :)

I hope I will not have forgotten that style next time I context-switch from XPCOM-land :)

Comment 5

[Jason Orendorff [:jorendorff]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #4)
> Actually, |Cleanup| cleans |p| and |p->rvalue|, which I need in the call to
> |ConvertToJS|. So, I suppose I could do that, but I would first need to copy
> |p->rvalue| and then clean that copy. Do you think it is useful?

Ah, no, don't bother.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Attachment: Returning the result of dispose

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Attachment: Returning the result of dispose

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Attachment: Returning the result of dispose

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce72679ffb95

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Attachment: Returning the result of dispose

Fixed testsuite build issue on Linux 64.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10b783e85c1e

The CTypes.cpp changes in the updated patch were already present in the file, FWIW.

Comment 12

[Richard Newman [:rnewman]]

Backed out whole merge for bustage per Yoric (Bug 743574):

https://hg.mozilla.org/integration/mozilla-inbound/rev/12e42fb8e321

Comment 13

[Richard Newman [:rnewman]]

Disregard that; PEBKAC. Did not get backed out. I misread TBPL.

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Attachment: Returning the result of dispose

Comment 15

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/ce72679ffb95

Comment 16

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/10b783e85c1e

Comment 17

[Matt Brubeck (:mbrubeck)]

Sorry, I backed out these changes because they depend on bug 720771 which was backed out because of crashes (bug 745233):
https://hg.mozilla.org/mozilla-central/rev/e1f0bb28fbb4

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Attachment: Returning the result of dispose

I _think_ that I have managed to fix the mysterious mysterious almost-always-reproducible Linux 64bit PGO test crash (bug 745233). I will keep attempting to find out what causes it, though.

Comment 19

[David Teller [:Yoric] - still alive but not very active]

Attachment: Returning the result of dispose

Applied same changes as in parent bug.

Comment 20

[David Teller [:Yoric] - still alive but not very active]

As jorendorff has marked the parent bug r+ and as I have made the exact same changes here, I will just proceed.

Comment 21

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 615714 [details] [diff] [review]
Returning the result of dispose

[Approval Request Comment]
Please see https://wiki.mozilla.org/Tree_Rules for information on mozilla-central landings that do not require approval.

Possible risk to Fennec Native 14 (if any): Patches js-ctypes to add new features. It should not alter the behavior of any existing feature, though.

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 615714 [details] [diff] [review]
Returning the result of dispose

[Triage Comment]
Merge of 14 to Aurora has completed.  Please renominate for mozilla-aurora.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/131687a69268

Comment 24

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/131687a69268