1193247 - Server side logging: optimize content data allocation

Status: RESOLVED WONTFIX

(DevTools :: Console, defect)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

When parsing server side logs (coming through HTTP headers) data are allocated in chrome scope (in ServerLoggingListener.parse() method) and later cloned into the content scope (in WebConsoleActor.onServerLogCall() method).

It means that the allocation is done twice while we could optimize and do it just once.

See also: https://bugzilla.mozilla.org/show_bug.cgi?id=1168872#c30


Honza

Comment 1

[Florent Fayolle]

Attachment: 1193247.patch

Something like this?

Or is there any better way to call JSON.parse in the context of the debuggee window?

Florent

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8654634 [details] [diff] [review]
1193247.patch

The attached patch is empty.

forgot to: hg qrefresh?


Btw. I thought you could work on bug 1193197, but having both fixed is even better :)

Also, Alexandre Poirot [:ochameau] has been doing all reviews for the server-logger (and also suggested these follow ups) so I think you should ask him for reviews...

Honza

Comment 3

[Florent Fayolle]

Hum... facepalm!

> Btw. I thought you could work on bug 1193197, but having both fixed is even better :)

Ah, right. I also take it.

> I think you should ask him for reviews...

Sure!

Florent

Comment 4

[Florent Fayolle]

Attachment: 1193247.patch

Alex,

Can you please review this patch?
I simply use a sandbox in order to serialize the JSON and get an object coming from the content context.

What do you think? Is there any smarter way to do this?

Florent

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 8654985 [details] [diff] [review]
1193247.patch

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

::: toolkit/devtools/webconsole/server-logger.js
@@ +275,5 @@
> +        sandbox.result = result;
> +        data = Cu.evalInSandbox("JSON.parse(result)", sandbox);
> +      } else {
> +        data = JSON.parse(result);
> +      }

I would just do:
  data = this.window.JSON.parse(result);
There is no need of an additional sandbox.

Also, is there some cases where this.window is undefined?

Comment 6

[Florent Fayolle]

> I would just do:
>  data = this.window.JSON.parse(result);

Yeah, I would have done that if it worked :/. Unfortunately, the |this.window.JSON.parse| method is undefined :

>>> this.window.JSON
Opaque {  }
>>> this.window.JSON.parse
undefined
XrayWrapper denied access to property parse (reason: object is not safely Xrayable). See https://developer.mozilla.org/en-US/docs/Xray_vision for more information. Note that only the first denied property access from a given global object will be reported.

Florent

Comment 7

[Alexandre Poirot [:ochameau]]

Sorry for the delay.

So the issue with using a sandbox is that it has a pretty significant cost by itself.
Also, the object created in the sandbox aren't exactly like webpage objects,
they are going to live in the sandbox compartment (that may introduce some wrapper usages)
and their global is the sandbox.

The Cu.cloneInto may end up being better than this sandbox trick.

(And Cu.waiveXrays(this.window).JSON.parse isn't a safe option either)

Comment 8

[Alexandre Poirot [:ochameau]]

Gabor, sorry to ping you about that, but you are my new xray expert ;)

Do you know why content.JSON.parse throws this?

  XrayWrapper denied access to property parse (reason: object is not safely Xrayable).
  See https://developer.mozilla.org/en-US/docs/Xray_vision for more information. 
  Note that only the first denied property access from a given global object will be reported.

Actually I don't really care to understand why, if you know a way to have a safe access to the webpage JSON object.
Our goal here is to create an object in the page compartment out of a JSON string.
Today, we create an object in system compartment and then use Cu.cloneInto,
but that sounds rather inefficient. I imagine it creates two copies of the object?

Comment 9

[Gabor Krizsanits (INACTIVE)]

Sorry for the delay Alex, I was on PTO...

(In reply to Alexandre Poirot [:ochameau] from comment #8)
> Do you know why content.JSON.parse throws this?

In short, parse is a callable and by default xrays on js objects filters it out.

> Our goal here is to create an object in the page compartment out of a JSON
> string.
> Today, we create an object in system compartment and then use Cu.cloneInto,
> but that sounds rather inefficient. I imagine it creates two copies of the
> object?

Yes cloning creates a second instance, and I don't think an addition sandbox
would make the situation any better. The proper solution here would be to
add xray support for JSON. Bobby, since we seem to have a native implementation
for JSON (http://mxr.mozilla.org/mozilla-central/source/js/src/json.cpp) I expect
this to be doable. Do we have any reason not to add JSProto_JSON to the list in
IsJSXraySupported? (http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/XrayWrapper.cpp?force=1#74)

Comment 10

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #9)
> Yes cloning creates a second instance, and I don't think an addition sandbox
> would make the situation any better. The proper solution here would be to
> add xray support for JSON. Bobby, since we seem to have a native
> implementation
> for JSON (http://mxr.mozilla.org/mozilla-central/source/js/src/json.cpp) I
> expect
> this to be doable. Do we have any reason not to add JSProto_JSON to the list
> in
> IsJSXraySupported?
> (http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/
> XrayWrapper.cpp?force=1#74)

We could support JSON over Xrays if it were needed for compat with existing scripts. However, it's worth noting that the semantics of JSXrays would cause it to behave identically with window.JSON.



(In reply to Alexandre Poirot [:ochameau] from comment #8)
> Actually I don't really care to understand why, if you know a way to have a
> safe access to the webpage JSON object.
> Our goal here is to create an object in the page compartment out of a JSON
> string.
> Today, we create an object in system compartment and then use Cu.cloneInto,
> but that sounds rather inefficient. I imagine it creates two copies of the
> object?

JSON.parse + cloneInto is going to be much, much more efficient than anything you might do over Xrays. And even if we supported content.JSON.parse, it still won't actually give you an object in the content compartment. So I think you're already doing the right thing here.

Comment 11

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #10)
> And even if we supported
> content.JSON.parse, it still won't actually give you an object in the
> content compartment.

How come? I would expect contentWindow.JSON.parse to create an object in the
contentWindow compartment.

Comment 12

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #11)
> (In reply to Bobby Holley (:bholley) from comment #10)
> > And even if we supported
> > content.JSON.parse, it still won't actually give you an object in the
> > content compartment.
> 
> How come? I would expect contentWindow.JSON.parse to create an object in the
> contentWindow compartment.

That's not how JSXrays work. JSXrays re-create the methods in the caller's compartment. If a method mints an object, there's no way for that object to know that it's supposed to be created in another compartment, and no real safe way to  do it.

Comment 13

[Gabor Krizsanits (INACTIVE)]

also per IRC:
<bholley> gabor: I think the behavior you're asking for violates the spec
<bholley> gabor: i.e. window[0].JSON.parse.call(window[1].JSON, "...")
<bholley> gabor: per-spec, I'm pretty sure that will create an object in window[0]

So my approach is not going to work.

Comment 14

[Alexandre Poirot [:ochameau]]

(In reply to Bobby Holley (:bholley) from comment #10) 
> JSON.parse + cloneInto is going to be much, much more efficient than
> anything you might do over Xrays.

Ok, ok. Thanks for the wrappers insight!

Jan, Florent, we will just keep it as-is until we see a real issue with server side performances.