Last Comment Bug 671113 - Poison JSScripts when freed to get more crash data
: Poison JSScripts when freed to get more crash data
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: Bill McCloskey (:billm)
:
Mentors:
Depends on:
Blocks: 670702
  Show dependency treegraph
 
Reported: 2011-07-12 16:35 PDT by Bill McCloskey (:billm)
Modified: 2011-07-20 06:57 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (8.14 KB, patch)
2011-07-12 16:35 PDT, Bill McCloskey (:billm)
dmandelin: review+
Details | Diff | Review
new patch (8.14 KB, patch)
2011-07-13 09:43 PDT, Bill McCloskey (:billm)
dmandelin: review+
Details | Diff | Review

Description Bill McCloskey (:billm) 2011-07-12 16:35:20 PDT
Created attachment 545522 [details] [diff] [review]
patch

This is a follow-up on bug 670702. This patch poisons scripts when they're freed. It also tries to verify that the circular list of scripts that each compartment keeps is valid. It adds a cookie field to the beginning and middle of each script, and checks that the cookies have the right values. I'm not exactly sure what we'll get out of this, but hopefully it will give us something useful.
Comment 1 David Mandelin [:dmandelin] 2011-07-12 18:22:13 PDT
Comment on attachment 545522 [details] [diff] [review]
patch

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

A name like js_CheckCompartmentScripts is probably better than js_AssertScriptListValid, since it's checking the validity of the scripts in the list, not the lists itself, but I think I don't care that much about a single diagnostic function. Now that I mention that, it should js::Whatever instead of js_Whatever, though.
Comment 2 Bill McCloskey (:billm) 2011-07-13 09:43:42 PDT
Created attachment 545679 [details] [diff] [review]
new patch

Sorry for the re-review. The previous patch timed out on tryserver in jstests. I guess that iterating over every script every time we make a new script is too expensive. So now that traversal only happens during a GC. I also added some new instrumentation to js_TraceScript.
Comment 3 David Mandelin [:dmandelin] 2011-07-13 09:53:41 PDT
Comment on attachment 545679 [details] [diff] [review]
new patch

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

::: js/src/jsscript.cpp
@@ +1408,5 @@
> +CheckScript(JSScript *script, JSScript *prev)
> +{
> +    if (script->cookie1 != JS_SCRIPT_COOKIE || script->cookie2 != JS_SCRIPT_COOKIE) {
> +        volatile char dbg1[sizeof(JSScript)], dbg2[sizeof(JSScript)];
> +        memcpy((void *)dbg1, script, sizeof(JSScript));

I suspect the optimizer will optimize away the |memcpy|s. I just tried it in an example program in MSVC10 and it did. You could test it easily enough and see if it does in this case.

In the past, I wrote my own memcpy with a volatile destination parameter, and that worked.
Comment 4 Bill McCloskey (:billm) 2011-07-13 10:23:35 PDT
OK, thanks Dave. I'll make sure it works before pushing.
Comment 5 Marco Bonardo [::mak] 2011-07-20 06:57:19 PDT
http://hg.mozilla.org/mozilla-central/rev/44f0f4395d6a

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