Closed Bug 900268 Opened 6 years ago Closed 6 years ago

Trace actor collects too much information when serializing objects

Categories

(DevTools :: General, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 25

People

(Reporter: rjacob, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

When serializing an object, the entire graph of reachable objects (via both ownProperties and the prototype) is serialized. This can result in huge amounts of data being sent over the remote debugging protocol on every function entry and exit. For example, `return {};` serializes Object.prototype and produces a 24,000 character packet: http://pastebin.mozilla.org/2696180

Some proposed solutions:

- Don't serialize prototypes. This shrinks packet sizes significantly for simple objects, but doesn't help with large data structure objects.

- Only serialize objects to a certain depth. This prevents large trees from being serialized, but doesn't solve the problem for objects that reference the global object, DOM nodes, or long arrays.

- Only serialize basic information about object parameters (like the class, possibly the list of names of ownProperties). This is similar to the previous suggestion, but with a max depth of 0. This solves the problem of individual large objects causing problematic packets, but makes the collected data not as useful.

The amount of information the user wants collected in the trace could potentially be an option--you'd trade performance for greater detail. Collecting everything (like we do now) is more detail than necessary for the majority of use cases, though.
Blocks: 887024
Attached patch Patch (obsolete) — Splinter Review
This patch serializes the ownProperties of argument and return value objects, but only creates an object grip for references in those objects (including the prototype reference). We could still get huge packets and slowdown when the arguments array is very long or when an object with a large ownProperties list is serialized...should we do something about that?

Example serialization output: http://pastebin.mozilla.org/2766675
Attachment #785095 - Flags: feedback?(rcampbell)
Attachment #785095 - Flags: feedback?(past)
Attachment #785095 - Flags: feedback?(nfitzgerald)
Forgot to remove a deleted test from xpcshell.ini.
Attachment #785095 - Attachment is obsolete: true
Attachment #785095 - Flags: feedback?(rcampbell)
Attachment #785095 - Flags: feedback?(past)
Attachment #785095 - Flags: feedback?(nfitzgerald)
Attachment #785105 - Flags: feedback?(rcampbell)
Attachment #785105 - Flags: feedback?(past)
Attachment #785105 - Flags: feedback?(nfitzgerald)
Comment on attachment 785095 [details] [diff] [review]
Patch

Review of attachment 785095 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, I think this is a practical trade off.
Attachment #785095 - Attachment is obsolete: false
Comment on attachment 785095 [details] [diff] [review]
Patch

Woops, old version of the patch
Attachment #785095 - Attachment is obsolete: true
Attachment #785105 - Flags: feedback?(nfitzgerald) → feedback+
Comment on attachment 785105 [details] [diff] [review]
serialization.diff

Review of attachment 785105 [details] [diff] [review]:
-----------------------------------------------------------------

tight
Attachment #785105 - Flags: feedback?(rcampbell) → feedback+
Attachment #785105 - Flags: feedback?(past) → review?(nfitzgerald)
Comment on attachment 785105 [details] [diff] [review]
serialization.diff

Review of attachment 785105 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nit fix below

::: toolkit/devtools/server/actors/tracer.js
@@ +581,3 @@
>   */
> +function objectGrip(aObject) {
> +  let g = {

Nit: Can we use `grip` instead of `g`. I like to avoid one letter variable names unless they are in a single map/filter or whatever and only used once.
Attachment #785105 - Flags: review?(nfitzgerald) → review+
This function is one of the ones copied from script.js. These duplicated functions are eventually going to be in only one place, so I made as few changes as possible to make the refactoring easier.
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/f98860de7350
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 25
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.