Last Comment Bug 765890 - Make tests pass when javascript.options.xml.content is off by default
: Make tests pass when javascript.options.xml.content is off by default
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: ---
Assigned To: general
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 761422 778851
  Show dependency treegraph
 
Reported: 2012-06-18 13:30 PDT by Jason Orendorff [:jorendorff]
Modified: 2012-09-04 12:15 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Partial (25.72 KB, patch)
2012-07-16 01:47 PDT, ted shroyer
no flags Details | Diff | Splinter Review
Patch for e4x tests (77.39 KB, patch)
2012-07-16 17:00 PDT, ted shroyer
no flags Details | Diff | Splinter Review
Strange errors related to having javascript.options.xml.content = false (13.94 KB, text/plain)
2012-07-16 17:02 PDT, ted shroyer
no flags Details
Patch using |reftest| header to run tests (241.93 KB, patch)
2012-07-18 19:47 PDT, ted shroyer
no flags Details | Diff | Splinter Review
Fixed mochitests and jsreftests (231.57 KB, patch)
2012-07-19 13:03 PDT, ted shroyer
no flags Details | Diff | Splinter Review
Patch with metadata for easier checkin (166.34 KB, patch)
2012-07-20 12:08 PDT, ted shroyer
jorendorff: review+
Details | Diff | Splinter Review
Alter mochitests/jsreftests to pass when javascript.options.xml.content defaults to false (151.93 KB, patch)
2012-07-23 17:01 PDT, ted shroyer
jorendorff: review+
Details | Diff | Splinter Review
Weirdness, part 1 - without SpecialPowers, things work (4.66 KB, patch)
2012-07-26 12:23 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
Weirdness, part 2 - with SpecialPowers, the pref doesn't take effect (3.66 KB, patch)
2012-07-26 12:25 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2012-06-18 13:30:32 PDT
This mostly requires some test suite work.
Comment 1 ted shroyer 2012-07-16 01:47:03 PDT
Created attachment 642506 [details] [diff] [review]
Partial

Fixes most of mochitest.
Omits jstests for e4x (need to add target to testing makefile)
Working through obvious errors in other js reftests
Comment 2 ted shroyer 2012-07-16 17:00:42 PDT
Created attachment 642794 [details] [diff] [review]
Patch for e4x tests

-Change to testing/testsuites-targets.mk to run e4x tests as a second pass (Concerned about the error reporting on the try servers)

-jsreftests changed to pass as many syntax related e4x calls as possible.  There are errors.  See attachement UnexpectedChanges.text
Comment 3 ted shroyer 2012-07-16 17:02:58 PDT
Created attachment 642795 [details]
Strange errors related to having javascript.options.xml.content = false
Comment 4 ted shroyer 2012-07-16 17:05:00 PDT
Interesting observations:
// 1) Crash when trying to resetPrefs() from javascript (may only happen in testing)
// While trying to save an alternate preference set and reload it
// Discovered this can make things crash
Services.prefs.savePrefFile(null);
Services.prefs.readUserPrefs(null);
Services.prefs.resetPrefs(); // Crashes on this line
// Not sure that the save and read are needed

// 2) runreftests.py parameter has strange behavior
// extra-profile-file did not work unless the file is user.js
runreftests.py --extra-profile-file=
Comment 5 ted shroyer 2012-07-18 19:47:09 PDT
Created attachment 643710 [details] [diff] [review]
Patch using |reftest| header to run tests
Comment 6 ted shroyer 2012-07-19 13:03:47 PDT
Created attachment 643984 [details] [diff] [review]
Fixed mochitests and jsreftests

(root)/js/src/tests/js1_6/Regress/regress-314887.js fails.  Needs work.
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-07-19 15:43:42 PDT
Ted, can you please make sure you patches have the needed metadata so that they're easier to eventually check in? Thanks!
https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in
Comment 8 ted shroyer 2012-07-20 12:08:46 PDT
Created attachment 644413 [details] [diff] [review]
Patch with metadata for easier checkin

Ryan, I think this patch has the metadata you needed for easier checkin.
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-07-20 14:07:53 PDT
(In reply to ted shroyer from comment #8)
> Created attachment 644413 [details] [diff] [review]
> Patch with metadata for easier checkin
> 
> Ryan, I think this patch has the metadata you needed for easier checkin.

Much better, thanks! One note, this patch is pretty self-explanatory based on the bug title, but typically you'll want a commit message that's succinctly describing what the patch is doing. You don't need to worry about that for this one, though.
Comment 10 Jason Orendorff [:jorendorff] 2012-07-20 15:52:46 PDT
Comment on attachment 644413 [details] [diff] [review]
Patch with metadata for easier checkin

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

r=me with a few minor changes, described below.

In these three files:
  content/xslt/tests/mochitest/test_bug427060.html
  content/xslt/tests/mochitest/test_bug440974.html
  content/xslt/tests/mochitest/test_bug453441.html
please use \n\ instead of plain \.

These are tests we are not going to delete when E4X goes, so I have a feeling it'd be rude of us to change the meaning of the code in these tests any more than we have to.

(The only way this would ever matter practically is that if the test ever suddenly started failing with a parse error, the parse error line number would be something meaningful instead of
just always being 1. So not a huge deal. It might never happen.)

In js/src/tests/js1_6/Regress/regress-314887.js:
>+// E4X NEEDS WORK
>+// javascript.options.xml.content = false causes this test to fail

Oops. This doesn't need debugging, it just needs the E4X pref turned on. (I told you it didn't, but I was wrong.)

> var BUGNUMBER = 314887;
> var summary = 'Do not crash when morons embed script tags in external script files';

Still my favorite bug summary ever.

::: js/src/tests/e4x/Regress/regress-474319.js
@@ +1,1 @@
> +// |reftest|  pref(javascript.options.xml.content,true) skip-if(Android)

Huh, you added one more space character here than in the other files. I don't care about the space, as long as it works.

::: js/xpconnect/tests/mochitest/test_bug618017.html
@@ +31,5 @@
> +    setAllowXML(default_allow_xml);
> +}
> +
> +setAllowXML(true);
> +var uneval_xml = uneval(eval('<test />'));

So gross.

But this is one of the tests we *do* want to delete, so it'll be all right.

If you want we can debug this and figure out why the pref doesn't take effect when we expected. I'm not super worried about it, but I'd like to know.

::: modules/libpref/src/init/all.js
@@ +682,4 @@
>  pref("javascript.options.pccounts.content", false);
>  pref("javascript.options.pccounts.chrome",  false);
>  pref("javascript.options.methodjit_always", false);
> +pref("javascript.options.xml.content", false);

This change shouldn't be landed at the same time as all the test fixes. The test fixes should land first, without toggling this pref. So please take it out of the patch.

Tomorrow or Tuesday we can send an updated patch to the Try server.
Comment 11 Jason Orendorff [:jorendorff] 2012-07-20 15:55:10 PDT
Ted mentioned that using pref() in js-tests causes js/src/tests/lib/manifest.py to spit out a few warnings.  The parser in there needs to be updated to cope with pref().  Regexp hacking.  :-P
Comment 12 ted shroyer 2012-07-23 17:01:50 PDT
Created attachment 645120 [details] [diff] [review]
Alter mochitests/jsreftests to pass when javascript.options.xml.content defaults to false

Should contain changes for jorendorff's comments and deal with recent removal of some test cases
Comment 13 Jason Orendorff [:jorendorff] 2012-07-26 10:45:32 PDT
Very pretty green Try Server run you probably can't see:
  https://tbpl.mozilla.org/?tree=Try&rev=eccf29eab7ab
Comment 14 Jason Orendorff [:jorendorff] 2012-07-26 12:23:29 PDT
Created attachment 646257 [details] [diff] [review]
Weirdness, part 1 - without SpecialPowers, things work

This patch shows that two existing tests can be made to pass even if the pref is off by default. You just have to turn it on first.

However it's really fragile and unpredictable. I think there's something wrong with the pref. The next patch will show one strange way to make the tests fail.
Comment 15 Jason Orendorff [:jorendorff] 2012-07-26 12:25:00 PDT
Created attachment 646259 [details] [diff] [review]
Weirdness, part 2 - with SpecialPowers, the pref doesn't take effect

Just switching from enablePrivilege to SpecialPowers is enough to make the pref not take effect. Very strange.
Comment 16 Jason Orendorff [:jorendorff] 2012-07-27 11:17:19 PDT
OK. There are multiple parts to the strangeness.

One weird thing is that `eval('<x/>')` is different from plain `<x/>`. From just a plain sanity standpoint this Should Not Be. The bug is that overrideVersion stomps on the language version and options. For direct eval code, that stuff should be inherited from the calling script regardless of what else is on the stack.

That's a bug. To me versionOverride is crazy from top to bottom; I can't imagine what that is supposed to be for.

But I say we land this patch and fix versionOverride later, perhaps after deleting E4X.
Comment 17 Jason Orendorff [:jorendorff] 2012-07-27 16:22:50 PDT
Comment on attachment 645120 [details] [diff] [review]
Alter mochitests/jsreftests to pass when javascript.options.xml.content defaults to false

The technique this patch uses to fix the two js/xpconnect mochitests is definitely sketchy. I don't see a better way, though, only other equally ugly workarounds. Let's do it. r=me.
Comment 18 Jason Orendorff [:jorendorff] 2012-07-30 10:44:03 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/baffbb5841f3
Comment 19 Jason Orendorff [:jorendorff] 2012-07-30 12:22:58 PDT
OK, this bug is about the test suite. The patch has landed.

Filed bug 778851 for the rest.
Comment 20 Jason Orendorff [:jorendorff] 2012-07-30 13:06:57 PDT
Follow-up fix:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/2ed8be848b93

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