Closed Bug 1453385 Opened 6 years ago Closed 6 years ago

Prevent protocol.js from copying all response objects

Categories

(DevTools :: Framework, enhancement)

enhancement
Not set
normal

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 2 open bugs)

Details

(Whiteboard: dt-fission)

Attachments

(7 files)

While working on bug 1449162 I saw that protocol.js is significantly slower than old fashion actors.
One reason is the duplication of all response objects done here:
  https://searchfox.org/mozilla-central/rev/6bfadf95b4a6aaa8bb3b2a166d6c3545983e179a/devtools/shared/protocol.js#712-727
  write: function(ret, ctx) {
    return JSON.parse(JSON.stringify(this.template, function(key, value) {
      if (value instanceof RetVal) {
        return value.write(ret, ctx);
      }
      return value;
    }));
  },

We don't want to mutate `this.template`, so we have to copy it,
but here we also copy the objects returned by actor implementations.
Ideally we would just hand it over to transport layer and have it transferred to the client as-is.
Assignee: nobody → poirot.alex
So the main fix is the last patch against protocol.js, but once this is done, I had to fix the animation actor that broke with this patch. Passing Infinity as-is was breaking two tests.
And in order to prevent object duplication, but at a lower level, right here:
  https://searchfox.org/mozilla-central/source/dom/base/nsFrameMessageManager.cpp#464
I had to remove the setFormProperty feature, which is now unused.
The message manager was still using JSON.parse/stringify because we were passing an object with a function,
and function can't be transferred across processes, so it falled back to JSON, which strips functions. (but also duplicates everyhing...)

So I'm expecting even better results form these patches than the one I reported from bug 1449162 comment 49, but let's see what try is going to say:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=6739d8923759c49b2ddc5f36b42c88917d8d8ae3&framework=1&selectedTimeRange=172800
(this is on top of m-c)

And another run, on top of bug 1449162:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=c351b5ad4846b4a8f27ade7cb2c1b489f5865cd4&newProject=try&newRevision=b0bdd01aa3bd147016ca7657ddededef58561849&framework=1
Could we also add some validation in p.js to check for these bad cases and error on them?  

For example, if an object with a function triggers a slow path in message manager like you mention, can we check for it so we don't add it accidentally in the future?

(Of course, we don't want such validation to be slow either, so we need to balance that also.  This just feels like a case where computers should be helping us...)
Flags: needinfo?(poirot.alex)
Comment on attachment 8967062 [details]
Bug 1453385 - Fix typo to prevent passing Infinity to protocol.js.

https://reviewboard.mozilla.org/r/235722/#review241504

Looks reasonable!
Attachment #8967062 - Flags: review?(jryans) → review+
Comment on attachment 8967063 [details]
Bug 1453385 - Prevent protocol.js from copying all response objects.

https://reviewboard.mozilla.org/r/235724/#review241580

This definitely seem worth doing, thanks for working on it! :) There are some error handling edge cases I'd like to see addressed, so marking r- for now.

::: devtools/shared/protocol.js:722
(Diff revision 1)
>     * @param object ctx
>     *    The object writing the response.
>     */
>    write: function(ret, ctx) {
> -    return JSON.parse(JSON.stringify(this.template, function(key, value) {
> +    // Consider that `template` is either directly a `RetVal`,
> +    // or a dictionnary with may be one `RetVal`.

Nit: "dictionary where each property may be a `RetVal`"

::: devtools/shared/protocol.js:731
(Diff revision 1)
> +    let result = {};
> +    for (let key in this.template) {
> +      let value = this.template[key];
>        if (value instanceof RetVal) {
> -        return value.write(ret, ctx);
> +        result[key] = value.write(ret, ctx);
> +      } else {

The previous version of this implicitly supported arbitrary levels of nesting, as in several layers of objects.  (It's not clear to me without reading every spec file if anyone actually used more than one level of object.)

Assuming we only need to support a single level of object for now, can we add a resonable error in case a property value is a plain object, maybe something about complex response templates aren't supported?

::: devtools/shared/protocol.js:732
(Diff revision 1)
> +    for (let key in this.template) {
> +      let value = this.template[key];
>        if (value instanceof RetVal) {
> -        return value.write(ret, ctx);
> +        result[key] = value.write(ret, ctx);
> +      } else {
> +        result[key] = value;

Do we actually expect a template that is an object with a property value that is _not_ `RetVal`?

If that's not an expected case, maybe we should be helpful and throw an error?
Attachment #8967063 - Flags: review?(jryans) → review-
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> Could we also add some validation in p.js to check for these bad cases and
> error on them?  
> 
> For example, if an object with a function triggers a slow path in message
> manager like you mention, can we check for it so we don't add it
> accidentally in the future?

I had to add such code to debug this.
The message manager code ends up only displaying a JS warning:
  "Sending message that cannot be cloned. Are you trying to send an XPCOM?"
  refering to the sendAsyncMessage call in transport.js
It wasn't useful for two reasons:
 * this message is displayed a bit after the call to the RDP method, not just right after, so that even if you print all packets, it is hard to say which one is the culprit,
 * it is a warning, so tests are passing.

Unfortunately, such check will be slow. I can't think of anything cheap.
The best check is to do what platform code do and do a StructuredClone.
On top of that, it is hard to do a StructuredClone from Sandboxes, I had to hack a random jsm to get it to work:
 * Add this before calling sendAsyncMessage:
   let { StructuredClone } = Cu.import(".../any.jsm", {});
   try {
     StructuredClone(packet);
    } catch(e) {
      dump(" Following packer isn't serializable: "+JSON.stringify(packet)+"\n");
    }
 * And put this in any JSM (because it is hard to export/steal StructuredCloneHolder from a JSM:
   var global = this;
   var StructuredClone = function StructuredClone(data) {
     let holder = new StructuredCloneHolder(data);
     holder.deserialize(global); 
   }

At the end it was only setFormAttribute function, but I thought it was more than just one case...
So I'm not sure it is worth spending ton of time detecting wrong serialization, it looks quite rare.
But I have to agree it would be nice to have a check while running tests.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> ::: devtools/shared/protocol.js:722
> (Diff revision 1)
> >     * @param object ctx
> >     *    The object writing the response.
> >     */
> >    write: function(ret, ctx) {
> > -    return JSON.parse(JSON.stringify(this.template, function(key, value) {
> > +    // Consider that `template` is either directly a `RetVal`,
> > +    // or a dictionnary with may be one `RetVal`.
> 
> Nit: "dictionary where each property may be a `RetVal`"

This particular method accepts more than one RetVal, but Response constructor will throw if there is more than one:
  https://searchfox.org/mozilla-central/source/devtools/shared/protocol.js#702
This is why my formulation is cryptic. I don't think I was really clear about that.

(sorry about the repetition of dictionary with two n in every patches!)

> ::: devtools/shared/protocol.js:731
> (Diff revision 1)
> > +    let result = {};
> > +    for (let key in this.template) {
> > +      let value = this.template[key];
> >        if (value instanceof RetVal) {
> > -        return value.write(ret, ctx);
> > +        result[key] = value.write(ret, ctx);
> > +      } else {
> 
> The previous version of this implicitly supported arbitrary levels of
> nesting, as in several layers of objects.  (It's not clear to me without
> reading every spec file if anyone actually used more than one level of
> object.)

Exactly. I tried to look at all usages of RetVal and did not found any >1 nested usage.

> Assuming we only need to support a single level of object for now, can we
> add a resonable error in case a property value is a plain object, maybe
> something about complex response templates aren't supported?

I imagine you may pass an object with constants, like traits.
I'll have to look at all `response` usages to see if there is any such scenario.
But we may only have two kinds of responses:
 * `response: RetVal("foo")`
 * `response: { attribute: RetVal("foo") }`
If that's the case, we can implement Response and its `template` attribute accordingly and reject anything else.

The more I look into protocol.js the more cryptic it is. It looks like there is a bunch of unused or very rarely used features. I would like to suggest removing everything that isn't used and also look into the things that are used only once.
An example of things that is complex and used once, from what I can see, is "details".
Which seems to be used only here:
  https://searchfox.org/mozilla-central/source/devtools/shared/specs/styles.js#33-39
Flags: needinfo?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #8)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> > Could we also add some validation in p.js to check for these bad cases and
> > error on them?  
> > 
> > For example, if an object with a function triggers a slow path in message
> > manager like you mention, can we check for it so we don't add it
> > accidentally in the future?
> At the end it was only setFormAttribute function, but I thought it was more
> than just one case...
> So I'm not sure it is worth spending ton of time detecting wrong
> serialization, it looks quite rare.
> But I have to agree it would be nice to have a check while running tests.

Maybe it's worth having at least in testing mode then?  We could check for the `testing` flag and only run the check if true.

> > Assuming we only need to support a single level of object for now, can we
> > add a resonable error in case a property value is a plain object, maybe
> > something about complex response templates aren't supported?
> 
> I imagine you may pass an object with constants, like traits.
> I'll have to look at all `response` usages to see if there is any such
> scenario.
> But we may only have two kinds of responses:
>  * `response: RetVal("foo")`
>  * `response: { attribute: RetVal("foo") }`
> If that's the case, we can implement Response and its `template` attribute
> accordingly and reject anything else.

Right.  Not aware of cases that set constants in the template...  Might be easier to block it with a useful error.

> The more I look into protocol.js the more cryptic it is. It looks like there
> is a bunch of unused or very rarely used features. I would like to suggest
> removing everything that isn't used and also look into the things that are
> used only once.
> An example of things that is complex and used once, from what I can see, is
> "details".
> Which seems to be used only here:
>  
> https://searchfox.org/mozilla-central/source/devtools/shared/specs/styles.
> js#33-39

Yes, there are some complex features that are infrequently used, especially around Inspector since dcamp added those and pulled out all the p.js tricks as the original author. :)

Happy to discuss more about cleaning up things to reduce complexity, improve perf, etc.
Comment on attachment 8967061 [details]
Bug 1453385 - Remove NodeActor custom form attributes.

https://reviewboard.mozilla.org/r/235720/#review241746

Looks reasonable to me, thanks Alex for the patch!

R+ assuming try is green

Honza
Attachment #8967061 - Flags: review?(odvarko) → review+
Depends on: 1453712
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #9)
> Maybe it's worth having at least in testing mode then?  We could check for
> the `testing` flag and only run the check if true.

So I did that in a new changeset, but flags.testing wasn't set in content process, so you will need bug 1453712 to have a correct behavior.
This debugging code helped me identifying other methods that return function or xpcom objects,
I fixed them in yet another patch in this queue ;)

> Right.  Not aware of cases that set constants in the template...  Might be
> easier to block it with a useful error.

I still have to address that.
Comment on attachment 8967585 [details]
Bug 1453385 - Make protocol.js throw if a spec implements a nested response object.

https://reviewboard.mozilla.org/r/236280/#review242054

> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #9)
> Right.  Not aware of cases that set constants in the template...  Might be
> easier to block it with a useful error.

Addressed in this independent patch.
I scanned the specs and could not find nested usages in response,
but let's see if try identifies one:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=68149abc86c3cc84c41637c3c14f4557fba55391
Comment on attachment 8967574 [details]
Bug 1453385 - Throw error when RDP message can't be serialized in message manager.

https://reviewboard.mozilla.org/r/236254/#review242066

Quite cool, thanks for adding this check! :)

::: commit-message-79e7b:3
(Diff revision 2)
> +Bug 1453385 - Throw error when RDP message can't be serialized in message manager. r=jryans
> +
> +If the packet contains a function or anything that StructureClone doesn't support,

Nit: Structure_d_

::: devtools/shared/builtin-modules.js:18
(Diff revision 2)
>   * they would also miss them.
>   */
>  
>  const { Cu, CC, Cc, Ci } = require("chrome");
>  const promise = require("resource://gre/modules/Promise.jsm").Promise;
> -const jsmScope = require("resource://gre/modules/Services.jsm");
> +const jsmScope = require("resource://devtools/shared/Loader.jsm");

Hmm...  Even more mystical that before, but if it works...

::: devtools/shared/builtin-modules.js:24
(Diff revision 2)
>  const { Services } = jsmScope;
>  // Steal various globals only available in JSM scope (and not Sandbox one)
>  const {
>    console,
>    HeapSnapshot,
> +  StructuredCloneHolder,

Could we instead add `StructuredCloneHolder` to the list for requestable global properties[1]?  Then you can retrieve it from the sandbox below.

[1]: https://searchfox.org/mozilla-central/rev/9f3da81290054c5b8955bb67ff98cae66676f745/js/xpconnect/src/Sandbox.cpp#787-860

::: devtools/shared/transport/transport.js:759
(Diff revision 2)
>        this.emit("close");
>        this.hooks.onClosed();
>      },
>  
>      receiveMessage: function({data}) {
>        this.emit("packet", data);

Seeing this line reminds me that this packet emitting stuff is dead code.  I filed bug 1453846.

::: devtools/shared/transport/transport.js:797
(Diff revision 2)
>        this.emit("send", packet);
> +      if (flags.testing && !this._canBeSerialized(packet)) {
> +        let attributes = this.pathToUnserializable(packet);
> +        let msg = "Following packet can't be serialized: " + JSON.stringify(packet);
> +        msg += "\nBecause of attributes: " + attributes.join(", ") + "\n";
> +        msg += "Did you passed a function or an XPCOM object in it?";

Nit: pass
Attachment #8967574 - Flags: review?(jryans) → review+
Comment on attachment 8967575 [details]
Bug 1453385 - Fix actors trying to transfer functions or xpcom objects.

https://reviewboard.mozilla.org/r/236256/#review242068

Very exciting to see some things be caught! :)
Attachment #8967575 - Flags: review?(jryans) → review+
Comment on attachment 8967063 [details]
Bug 1453385 - Prevent protocol.js from copying all response objects.

https://reviewboard.mozilla.org/r/235724/#review242070

Looks great; thanks for all the extra correctness checks! :)

::: devtools/shared/protocol.js:722
(Diff revision 3)
>     * @param object ctx
>     *    The object writing the response.
>     */
>    write: function(ret, ctx) {
> -    return JSON.parse(JSON.stringify(this.template, function(key, value) {
> +    // Consider that `template` is either directly a `RetVal`,
> +    // or a dictionnary with may be one `RetVal`.

Nit: dictionary ;)
Attachment #8967063 - Flags: review?(jryans) → review+
Comment on attachment 8967585 [details]
Bug 1453385 - Make protocol.js throw if a spec implements a nested response object.

https://reviewboard.mozilla.org/r/236280/#review242072
Attachment #8967585 - Flags: review?(jryans) → review+
Blocks: 1453915
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #26)
> Nit: dictionary ;)

Should I introduce a new eslint for typical french to english typos?!

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #24)
> ::: devtools/shared/builtin-modules.js:24
> (Diff revision 2)
> >  const { Services } = jsmScope;
> >  // Steal various globals only available in JSM scope (and not Sandbox one)
> >  const {
> >    console,
> >    HeapSnapshot,
> > +  StructuredCloneHolder,
> 
> Could we instead add `StructuredCloneHolder` to the list for requestable
> global properties[1]?  Then you can retrieve it from the sandbox below.

Propably, I imagine it would be similar for Heapsnapshot and may be console also.

https://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/RegisterWorkerBindings.cpp#239
https://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/RegisterWorkerBindings.cpp#143

I opened bug 1453915 to followup on that.

Otherwise, I pushed yet another changeset to fix the couple of specs that were using something else than RetVal in response object.
Comment on attachment 8967678 [details]
Bug 1453385 - Fix actors using something else than RetVal in response's spec.

https://reviewboard.mozilla.org/r/236406/#review242230

Once again, great to see some stricter checking is helping us out!

::: devtools/server/performance/memory.js:79
(Diff revision 1)
>     */
>    attach: expectState("detached", function() {
>      this.dbg.addDebuggees();
>      this.dbg.memory.onGarbageCollection = this._onGarbageCollection.bind(this);
>      this.state = "attached";
> +    return "attached";

Maybe it's better to return `this.state` rather than retyping the string again...?  Whatever you prefer, though.

::: devtools/shared/specs/performance.js:82
(Diff revision 1)
>        response: { config: RetVal("json") }
>      },
>  
>      setProfilerStatusInterval: {
>        request: { interval: Arg(0, "number") },
> -      response: { oneway: true }
> +      oneway: true

Whoa, that's a great find!
Attachment #8967678 - Flags: review?(jryans) → review+
Comment on attachment 8967678 [details]
Bug 1453385 - Fix actors using something else than RetVal in response's spec.

https://reviewboard.mozilla.org/r/236406/#review242304

::: devtools/server/tests/unit/test_protocol_children.js:84
(Diff revision 2)
>      getSibling: {
>        request: { id: Arg(0) },
>        response: { sibling: RetVal("childActor") }
>      },
>      emitEvents: {
> -      response: { value: "correct response" },
> +      response: { value: RetVal("string") },

I missed this test and just fixed it.
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b8ac8bdb082c
Prevent protocol.js from copying all response objects. r=jryans
https://hg.mozilla.org/integration/autoland/rev/7e89bdce62bb
Make protocol.js throw if a spec implements a nested response object. r=jryans
https://hg.mozilla.org/integration/autoland/rev/767f2782c8fe
Remove NodeActor custom form attributes. r=Honza
https://hg.mozilla.org/integration/autoland/rev/565f6eac204d
Fix typo to prevent passing Infinity to protocol.js. r=jryans
https://hg.mozilla.org/integration/autoland/rev/c611078b5add
Throw error when RDP message can't be serialized in message manager. r=jryans
https://hg.mozilla.org/integration/autoland/rev/49862a0820f9
Fix actors trying to transfer functions or xpcom objects. r=jryans
https://hg.mozilla.org/integration/autoland/rev/a7561379843f
Fix actors using something else than RetVal in response's spec. r=jryans
Blocks: dt-pjs
Product: Firefox → DevTools
Whiteboard: dt-fission
You need to log in before you can comment on or make changes to this bug.