611462 - Abort at startup, after installing Adblock Plus in debug build: "Assertion failure: OptionsHasXML(options) == VersionHasXML(version), at ../../../mozilla/js/src/jsapi.cpp:1027"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Daniel Holbert [:dholbert]]

STEPS TO REPRODUCE:
 1. Create a debug mozilla-central build.
 2. Visit https://addons.mozilla.org/en-US/firefox/addon/1865/ and click "Add to Firefox"
 3. Click through the dialogs, and restart when prompted.

ACTUAL RESULTS:
Immediate abort with:
> Assertion failure: OptionsHasXML(options) == VersionHasXML(version), at ../../../mozilla/js/src/jsapi.cpp:1027

NOTE: This affects both my libxul and my non-libxul build. (not surprising)

I'll post a backtrace in a minute.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: backtrace

Comment 2

[Chris Leary [:cdleary] (not checking bugmail)]

We clearly don't have very good version-changing test coverage. I'll try to characterize this failure tonight -- should build up a small suite of version tests while I'm at it.

Comment 3

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: WIP: fix bad version behavior

The assertion failed because we were copying the flags from the former version into the new version, instead of from the current options to the new version. When you set options from the API, you're indicating what compilation options you'd like for *future* scripts, so it may be different from the currently executing script version's flags.

Need to add tests.

Comment 4

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Fix bad JS_SetVersion behavior.

The test demonstrates the failure mode and the patch resolves it.

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 490336 [details] [diff] [review]
Fix bad JS_SetVersion behavior.

r+, only nits:

>-    void clearVersionOverride() { hasVersionOverride = false; }
>+    void clearVersionOverride() {
>+        hasVersionOverride = false;
>+    }
>+    
>     bool isVersionOverridden() const { return hasVersionOverride; }
> 
>     /* Set the default script compilation version. */
>-    void setDefaultVersion(JSVersion version) { defaultVersion = version; }
>+    void setDefaultVersion(JSVersion version) {
>+        defaultVersion = version;
>+    }

If you are going to make clearVersionOverride and setDefaultVersion not all on one line, could you do the same for isVersionOverridden?

>+    void optionFlagsToVersion(JSVersion *version) {
>+        js::VersionSetXML(version, js::OptionsHasXML(options));
>+    }

Could you make this a const memfun?

Comment 6

[Chris Leary [:cdleary] (not checking bugmail)]

Nits addressed! http://hg.mozilla.org/tracemonkey/rev/9420a20e5e5b

Comment 7

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/9420a20e5e5b