Closed Bug 661903 Opened 11 years ago Closed 11 years ago

Move script filename table to the compartment


(Core :: JavaScript Engine, defect)

Not set





(Reporter: billm, Assigned: billm)




(1 file)

Attached patch patchSplinter Review
This is the last thing to move to the compartment. Once it's done, everything swept by the GC will be compartment-local. This will make it easier for all GCs to be compartment GCs.

Igor, there's one thing I don't understand about this code. Why do we mark all the filenames if gcKeepAtoms is true? This behavior seems to have been introduced in bug 291312. I'm guessing this code is needed if we create a script and GC before the script is rooted. It would be nice to take it out if it's not needed, or maybe fix it in a more elegant way (by rooting the script). There are a lot of uses of gcKeepAtoms, and most of them don't seem to be related to script filenames, as far as I can tell.
Attachment #537198 - Flags: review?(igor)
Comment on attachment 537198 [details] [diff] [review]

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

r+ with the comment below addressed.

As regarding removal of comp->rt->gcKeepAtoms checks lets file a new bug about it. I think you are right and this is not necessary, but lets keep the issues separated.

::: js/src/jsscript.cpp
@@ +793,5 @@
> +    ScriptFilenameTable::AddPtr p = comp->scriptFilenameTable.lookupForAdd(filename);
> +    if (!p) {
> +        size_t size = offsetof(ScriptFilenameEntry, filename) + strlen(filename) + 1;
> +        ScriptFilenameEntry *entry = (ScriptFilenameEntry *) OffTheBooks::malloc_(size);

Use cx->malloc_ here as we allocate memory that can only be released during the GC. This also removes the call to JS_ReportOutOfMemory(cx);

@@ +838,4 @@
>  }
>  void
> +js_MarkScriptFilenames(JSCompartment *comp)

Remove this function. Instead in comp->rt->gcKeepAtoms free the entry only when comp->rt->gcKeepAtoms is false.
Attachment #537198 - Flags: review?(igor) → review+
Duplicate of this bug: 567202
Assignee: general → wmccloskey
Whiteboard: [inbound]
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Depends on: 643967
You need to log in before you can comment on or make changes to this bug.