Last Comment Bug 774706 - weird dead xpc_UnmarkGrayScript floating around in nsJSContext::Serialize
: weird dead xpc_UnmarkGrayScript floating around in nsJSContext::Serialize
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: :Benjamin Peterson
: Andrew Overholt [:overholt]
Depends on:
  Show dependency treegraph
Reported: 2012-07-17 09:10 PDT by Luke Wagner [:luke]
Modified: 2012-08-08 09:33 PDT (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

kill dead xpc thing (847 bytes, patch)
2012-08-07 10:20 PDT, :Benjamin Peterson
no flags Details | Diff | Splinter Review
fix dead code (1018 bytes, patch)
2012-08-07 12:21 PDT, :Benjamin Peterson
continuation: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2012-07-17 09:10:07 PDT
Go figure:

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 2 Luke Wagner [:luke] 2012-07-17 09:28:06 PDT
I see.  bz: do you know how that stream serialization relates to the startup cache?
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2012-07-17 09:34:51 PDT
Pretty directly, last I checked: the result is what's stored in the startup cache.
Comment 4 Luke Wagner [:luke] 2012-07-17 09:38:58 PDT
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 [:bz] (still a bit busy) 2012-07-17 10:45:08 PDT
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] 2012-07-17 10:48:22 PDT
Ah, thanks for explaining.  Would you say that <script> tags are the majority (size-wise) of XDR'd scripts?
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2012-07-17 11:48:58 PDT
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] 2012-07-17 12:04:43 PDT
(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 2012-08-07 10:20:12 PDT
Created attachment 649684 [details] [diff] [review]
kill dead xpc thing

Going back to the original bug.
Comment 10 Luke Wagner [:luke] 2012-08-07 10:23:32 PDT
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:
Comment 11 :Benjamin Peterson 2012-08-07 10:27:54 PDT
Perhaps smaug can comment.
Comment 12 Andrew McCreight [:mccr8] 2012-08-07 10:36:53 PDT
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] 2012-08-07 11:07:56 PDT
I don't know what WriteScript does... but looks like it ends up calling
JS_EncodeInterpretedFunction or JS_EncodeScript.
Comment 14 :Benjamin Peterson 2012-08-07 11:52:26 PDT
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] 2012-08-07 11:53:30 PDT
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 2012-08-07 12:21:50 PDT
Created attachment 649741 [details] [diff] [review]
fix dead code
Comment 17 Andrew McCreight [:mccr8] 2012-08-07 13:18:44 PDT
Comment on attachment 649741 [details] [diff] [review]
fix dead code

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

Comment 19 Ed Morley [:emorley] 2012-08-08 09:33:35 PDT

Note You need to log in before you can comment on or make changes to this bug.