Last Comment Bug 742384 - In CDataFinalizer, .dispose() should return something useful
: In CDataFinalizer, .dispose() should return something useful
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: js-ctypes (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: David Teller [:Yoric] (please use "needinfo")
:
Mentors:
Depends on: ctypes.finalize
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-04 08:18 PDT by David Teller [:Yoric] (please use "needinfo")
Modified: 2012-04-25 07:32 PDT (History)
5 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Returning the result during dispose (4.38 KB, patch)
2012-04-04 09:12 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Returning the value after dispose (17.34 KB, patch)
2012-04-10 05:53 PDT, David Teller [:Yoric] (please use "needinfo")
jorendorff: review+
Details | Diff | Splinter Review
Returning the result of dispose (150 bytes, patch)
2012-04-10 14:59 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Returning the result of dispose (150 bytes, patch)
2012-04-11 01:45 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Returning the result of dispose (17.48 KB, patch)
2012-04-11 01:46 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Returning the result of dispose (17.55 KB, patch)
2012-04-11 06:05 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Returning the result of dispose (17.76 KB, patch)
2012-04-12 02:13 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Returning the result of dispose (22.74 KB, patch)
2012-04-13 15:00 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Returning the result of dispose (19.92 KB, patch)
2012-04-17 07:41 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review

Description David Teller [:Yoric] (please use "needinfo") 2012-04-04 08:18:04 PDT
At the moment, .dispose() returns undefined. It should return the result of the call to the finalizer.
Comment 1 David Teller [:Yoric] (please use "needinfo") 2012-04-04 09:12:27 PDT
Created attachment 612220 [details] [diff] [review]
Returning the result during dispose
Comment 2 David Teller [:Yoric] (please use "needinfo") 2012-04-10 05:53:07 PDT
Created attachment 613564 [details] [diff] [review]
Returning the value after dispose

Here is a new version. It simplifies the code a little and adds tests.
Comment 3 Jason Orendorff [:jorendorff] 2012-04-10 08:17:31 PDT
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] (please use "needinfo") 2012-04-10 10:57:27 PDT
(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] 2012-04-10 13:48:25 PDT
(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] (please use "needinfo") 2012-04-10 14:59:38 PDT
Created attachment 613786 [details] [diff] [review]
Returning the result of dispose
Comment 7 David Teller [:Yoric] (please use "needinfo") 2012-04-11 01:45:52 PDT
Created attachment 613908 [details] [diff] [review]
Returning the result of dispose
Comment 8 David Teller [:Yoric] (please use "needinfo") 2012-04-11 01:46:59 PDT
Created attachment 613909 [details] [diff] [review]
Returning the result of dispose
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-04-11 05:04:32 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce72679ffb95
Comment 10 David Teller [:Yoric] (please use "needinfo") 2012-04-11 06:05:44 PDT
Created attachment 613962 [details] [diff] [review]
Returning the result of dispose

Fixed testsuite build issue on Linux 64.
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-04-11 15:03:14 PDT
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] 2012-04-11 15:37:55 PDT
Backed out whole merge for bustage per Yoric (Bug 743574):

https://hg.mozilla.org/integration/mozilla-inbound/rev/12e42fb8e321
Comment 13 Richard Newman [:rnewman] 2012-04-11 17:54:31 PDT
Disregard that; PEBKAC. Did not get backed out. I misread TBPL.
Comment 14 David Teller [:Yoric] (please use "needinfo") 2012-04-12 02:13:48 PDT
Created attachment 614301 [details] [diff] [review]
Returning the result of dispose
Comment 17 Matt Brubeck (:mbrubeck) 2012-04-13 10:42:52 PDT
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] (please use "needinfo") 2012-04-13 15:00:31 PDT
Created attachment 614922 [details] [diff] [review]
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] (please use "needinfo") 2012-04-17 07:41:20 PDT
Created attachment 615714 [details] [diff] [review]
Returning the result of dispose

Applied same changes as in parent bug.
Comment 20 David Teller [:Yoric] (please use "needinfo") 2012-04-22 01:24:02 PDT
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] (please use "needinfo") 2012-04-23 02:31:56 PDT
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 2012-04-24 11:24:30 PDT
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] 2012-04-24 17:03:23 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/131687a69268

Note You need to log in before you can comment on or make changes to this bug.