719295 - clean up telemetry JSObject construction

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Nathan Froyd [:froydnj]]

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.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: patch

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?

Comment 2

[Luke Wagner [: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

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

&& at end of line, and s/js::AutoObjectRooter/JS::AutoObjectRooter/g

Comment 4

[Nathan Froyd [:froydnj]]

(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.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: patch

Thanks for the feedback!  Here's an updated patch.

Comment 6

[(dormant account)]

Comment on attachment 602065 [details] [diff] [review]
patch

Why are you changing my perfect code?

Comment 7

[Mozilla RelEng Bot]

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

Comment 8

[Mozilla RelEng Bot]

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/[e-mail]-1e8f061c3f4a

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/1c238e6cf7a1 - -u xpcshell would have been good.

Comment 11

[Nathan Froyd [:froydnj]]

Attachment: patch

Whoops, xpcshell tests were busted because of missing parens in ReflectHistogramAndSamples.

Comment 12

[Mozilla RelEng Bot]

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

Comment 13

[Mozilla RelEng Bot]

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/[e-mail]-5929be9e7beb

Comment 14

[Nathan Froyd [:froydnj]]

The Linux opt xpcshell failure looks like some weird infrastructure problem; xpcshell tests passed everywhere else.  Calling this one good.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d9e5dd1709e

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5d9e5dd1709e