The default bug view has changed. See this FAQ.

weird dead xpc_UnmarkGrayScript floating around in nsJSContext::Serialize

RESOLVED FIXED in mozilla17

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: luke, Assigned: Benjamin)

Tracking

unspecified
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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...
I'm afraid we seem to: <http://mxr.mozilla.org/mozilla-central/source/content/xul/content/src/nsXULElement.cpp?mark=2897-2897#2878>.
(Reporter)

Comment 2

5 years ago
I see.  bz: do you know how that stream serialization relates to the startup cache?
Pretty directly, last I checked: the result is what's stored in the startup cache.
(Reporter)

Comment 4

5 years ago
Interesting, so is this for a set of scripts disjoint from those XDR'd via (Read|Write)CachedScript (by mozJSComponentLoader::GlobalForLocation and mozJSSubScriptLoader::LoadSubScript) ?
Yes, this is for <script> tags in XUL documents.  Specifically for inline <script>s, for the callsite linked in comment 1, I believe.
(Reporter)

Comment 6

5 years ago
Ah, thanks for explaining.  Would you say that <script> tags are the majority (size-wise) of XDR'd scripts?
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.
(Reporter)

Comment 8

5 years ago
(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.)
(Assignee)

Comment 9

5 years ago
Created attachment 649684 [details] [diff] [review]
kill dead xpc thing

Going back to the original bug.
Assignee: nobody → bpeterson
Attachment #649684 - Flags: review?(luke)
(Reporter)

Comment 10

5 years ago
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?
Attachment #649684 - Flags: review?(luke) → review?(sphink)
(Assignee)

Comment 11

5 years ago
Perhaps smaug can comment.
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.
I don't know what WriteScript does... but looks like it ends up calling
JS_EncodeInterpretedFunction or JS_EncodeScript.
(Assignee)

Comment 14

5 years ago
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.
That would be the easiest thing to do.  I can't imagine the overhead of a function call will impact performance much here.
(Assignee)

Comment 16

5 years ago
Created attachment 649741 [details] [diff] [review]
fix dead code
Attachment #649684 - Attachment is obsolete: true
Attachment #649684 - Flags: review?(sphink)
Attachment #649741 - Flags: review?(continuation)
Comment on attachment 649741 [details] [diff] [review]
fix dead code

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

Thanks.
Attachment #649741 - Flags: review?(continuation) → review+
(Assignee)

Comment 18

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b25596aab946
https://hg.mozilla.org/mozilla-central/rev/b25596aab946
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.