774706 - weird dead xpc_UnmarkGrayScript floating around in nsJSContext::Serialize

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Luke Wagner [:luke]]

Go figure:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsJSEnvironment.cpp#1984

Related question: do we ever call this function?  It ends up being 1 of 2 uses of JS engine XDR, so it would be awesome if we could kill it...

Comment 1

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

I'm afraid we seem to: <http://mxr.mozilla.org/mozilla-central/source/content/xul/content/src/nsXULElement.cpp?mark=2897-2897#2878>.

Comment 2

[Luke Wagner [:luke]]

I see.  bz: do you know how that stream serialization relates to the startup cache?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Pretty directly, last I checked: the result is what's stored in the startup cache.

Comment 4

[Luke Wagner [:luke]]

Interesting, so is this for a set of scripts disjoint from those XDR'd via (Read|Write)CachedScript (by mozJSComponentLoader::GlobalForLocation and mozJSSubScriptLoader::LoadSubScript) ?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Yes, this is for <script> tags in XUL documents.  Specifically for inline <script>s, for the callsite linked in comment 1, I believe.

Comment 6

[Luke Wagner [:luke]]

Ah, thanks for explaining.  Would you say that <script> tags are the majority (size-wise) of XDR'd scripts?

Comment 7

[Boris Zbarsky [:bzbarsky]]

I would think not, because the inline scripts should generally be small (and perhaps even rare) and <script src> uses a different codepath here, right?

We could measure, if desired.

Comment 8

[Luke Wagner [:luke]]

(In reply to Boris Zbarsky (:bz) from comment #7)
Ah, good to hear; we'll measure.  (This is all in the context of removing the decompiler and bug 761723.)

Comment 9

[:Benjamin Peterson]

Attachment: kill dead xpc thing

Going back to the original bug.

Comment 10

[Luke Wagner [:luke]]

Comment on attachment 649684 [details] [diff] [review]
kill dead xpc thing

Removing them is one option, but I assume it was put here for a reason:
  http://hg.mozilla.org/mozilla-central/rev/defb50779980
Steve?

Comment 11

[:Benjamin Peterson]

Perhaps smaug can comment.

Comment 12

[Andrew McCreight [:mccr8]]

The question is, what does WriteScript do with aScriptObject?  If it can create any new references to aScriptObject anywhere in the heap, then we need to do an xpc_UnmarkGrayScript() before we do WriteScript.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

I don't know what WriteScript does... but looks like it ends up calling
JS_EncodeInterpretedFunction or JS_EncodeScript.

Comment 14

[:Benjamin Peterson]

I don't believe JS_EncodeScript currently can keep extra references around. Maybe it would be safer to retain the xpc_UnmarkGrayScript just in case.

Comment 15

[Andrew McCreight [:mccr8]]

That would be the easiest thing to do.  I can't imagine the overhead of a function call will impact performance much here.

Comment 16

[:Benjamin Peterson]

Attachment: fix dead code

Comment 17

[Andrew McCreight [:mccr8]]

Comment on attachment 649741 [details] [diff] [review]
fix dead code

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

Thanks.

Comment 18

[:Benjamin Peterson]

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

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b25596aab946