Closed Bug 719295 Opened 8 years ago Closed 8 years ago

clean up telemetry JSObject construction

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: froydnj, Assigned: froydnj)

Details

Attachments

(1 file, 2 obsolete files)

We have a mixture of coding styles for constructing JSObjects and storing properties.  Sometimes we do:

 JSObject *obj = ...
 JSObject *prop = ...
 // Step 1:
 JS_DefineProperty(..., obj, ..., prop...)
 // Step 2: fill in prop in some way

and sometimes we reverse the two steps.

Similarly, we sometimes do:

 JSObject *obj = ...
 // Step 1: fill in the return value of the function
 *ret = obj
 // Step 2: fill in obj in some way

or vice versa.

We should always do fill in objects before assigning them to their owner; doing things in that order avoids having partially constructed data in case of errors.

For GC concerns with the sole reference to objects living on the stack or registers, we should be using AutoObjectRooter more pervasively.
Attached patch patch (obsolete) — Splinter Review
Here's a straightforward patch to handle most of this.  Requesting feedback from a JS-person-in-the-know, though; is all the AutoObjectRooter stuff still necessary with the GC stack scanning in place?
Attachment #601950 - Flags: feedback?(luke)
Comment on attachment 601950 [details] [diff] [review]
patch

There are plans to remove the conservative stack scanner, so AutoObjectRooter is a good idea.

>-  return REFLECT_OK;
>+ 
>+ return REFLECT_OK;

Need an extra indent space
Attachment #601950 - Flags: feedback?(luke) → feedback+
&& at end of line, and s/js::AutoObjectRooter/JS::AutoObjectRooter/g
(In reply to Ms2ger from comment #3)
> && at end of line, and s/js::AutoObjectRooter/JS::AutoObjectRooter/g

I'm going to do the last and not the first; for whatever reason, the && condition style is used everywhere in this file.  (I personally find it more readable, but I am willing to be overruled by the style guide.)

I'll mark switching over as a low-priority todo.
Attached patch patch (obsolete) — Splinter Review
Thanks for the feedback!  Here's an updated patch.
Assignee: nobody → nfroyd
Attachment #601950 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #602065 - Flags: review?(taras.mozilla)
Comment on attachment 602065 [details] [diff] [review]
patch

Why are you changing my perfect code?
Attachment #602065 - Flags: review?(taras.mozilla) → review+
Whiteboard: [autoland-try:-b do -p all -u none -t none]
Whiteboard: [autoland-try:-b do -p all -u none -t none] → [autoland-in-queue]
Autoland Patchset:
	Patches: 602065
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=1e8f061c3f4a
Try run started, revision 1e8f061c3f4a. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=1e8f061c3f4a
Try run for 1e8f061c3f4a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=1e8f061c3f4a
Results (out of 14 total builds):
    success: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-1e8f061c3f4a
Whiteboard: [autoland-in-queue]
Keywords: checkin-needed
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/1c238e6cf7a1 - -u xpcshell would have been good.
Attached patch patchSplinter Review
Whoops, xpcshell tests were busted because of missing parens in ReflectHistogramAndSamples.
Attachment #602065 - Attachment is obsolete: true
Attachment #602340 - Flags: review+
Whiteboard: [autoland-try:-b do -p all -u xpcshell -t none]
Whiteboard: [autoland-try:-b do -p all -u xpcshell -t none] → [autoland-in-queue]
Autoland Patchset:
	Patches: 602340
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=5929be9e7beb
Try run started, revision 5929be9e7beb. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=5929be9e7beb
Try run for 5929be9e7beb is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5929be9e7beb
Results (out of 28 total builds):
    success: 25
    warnings: 2
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-5929be9e7beb
Whiteboard: [autoland-in-queue]
The Linux opt xpcshell failure looks like some weird infrastructure problem; xpcshell tests passed everywhere else.  Calling this one good.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5d9e5dd1709e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.