The default bug view has changed. See this FAQ.

In CDataFinalizer, .dispose() should return something useful

RESOLVED FIXED in mozilla15

Status

()

Core
js-ctypes
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

({dev-doc-needed})

unspecified
mozilla15
x86
Mac OS X
dev-doc-needed
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

At the moment, .dispose() returns undefined. It should return the result of the call to the finalizer.
Created attachment 612220 [details] [diff] [review]
Returning the result during dispose
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.
Assignee: nobody → dteller
Attachment #612220 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #613564 - Flags: review?(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! :)
Attachment #613564 - Flags: review?(jorendorff) → review+
(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 :)
(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.
Created attachment 613786 [details] [diff] [review]
Returning the result of dispose
Attachment #613564 - Attachment is obsolete: true
Keywords: checkin-needed, dev-doc-needed
Created attachment 613908 [details] [diff] [review]
Returning the result of dispose
Attachment #613786 - Attachment is obsolete: true
Created attachment 613909 [details] [diff] [review]
Returning the result of dispose
Attachment #613908 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce72679ffb95
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla14
Created attachment 613962 [details] [diff] [review]
Returning the result of dispose

Fixed testsuite build issue on Linux 64.
Attachment #613909 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/10b783e85c1e

The CTypes.cpp changes in the updated patch were already present in the file, FWIW.
Keywords: checkin-needed
Backed out whole merge for bustage per Yoric (Bug 743574):

https://hg.mozilla.org/integration/mozilla-inbound/rev/12e42fb8e321
Target Milestone: mozilla14 → ---
Keywords: checkin-needed
Disregard that; PEBKAC. Did not get backed out. I misread TBPL.
Keywords: checkin-needed
Target Milestone: --- → mozilla14
Created attachment 614301 [details] [diff] [review]
Returning the result of dispose
Attachment #613962 - Attachment is obsolete: true
Attachment #614301 - Attachment is obsolete: true
Attachment #613962 - Attachment is obsolete: false
https://hg.mozilla.org/mozilla-central/rev/ce72679ffb95
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/mozilla-central/rev/10b783e85c1e
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla14 → ---
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.
Attachment #613962 - Attachment is obsolete: true
Created attachment 615714 [details] [diff] [review]
Returning the result of dispose

Applied same changes as in parent bug.
Attachment #614922 - Attachment is obsolete: true
Attachment #615714 - Flags: review?(jorendorff)
As jorendorff has marked the parent bug r+ and as I have made the exact same changes here, I will just proceed.
Keywords: checkin-needed
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.
Attachment #615714 - Flags: review?(jorendorff) → approval-mozilla-central?
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.
Attachment #615714 - Flags: approval-mozilla-central?
https://hg.mozilla.org/integration/mozilla-inbound/rev/131687a69268
Keywords: checkin-needed
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/131687a69268
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.