Closed Bug 1073853 Opened 5 years ago Closed 5 years ago

Various Symbol-related test failures when Gecko 35 merges to Aurora

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla35
Tracking Status
firefox34 --- unaffected
firefox35 --- verified

People

(Reporter: RyanVM, Assigned: jorendorff)

References

Details

Attachments

(1 file, 3 obsolete files)

Need to account for Symbols being enabled only on trunk.

https://tbpl.mozilla.org/php/getParsedLog.php?id=49028577&tree=Try

20:35:01  WARNING -  TEST-UNEXPECTED-FAIL | tests/jit-test/jit-test/tests/gc/bug-1053676.js | /builds/slave/test/build/tests/jit-test/jit-test/tests/gc/bug-1053676.js:8:0 ReferenceError: Symbol is not defined (code 3, args "--ion-eager")
20:35:01     INFO -  INFO exit-status     : 3
20:35:01     INFO -  INFO timed-out       : False
20:35:01     INFO -  INFO stderr         2> /builds/slave/test/build/tests/jit-test/jit-test/tests/gc/bug-1053676.js:8:0 ReferenceError: Symbol is not defined

20:44:24  WARNING -  TEST-UNEXPECTED-FAIL | tests/jit-test/jit-test/tests/ion/dce-with-rinstructions.js | /builds/slave/test/build/tests/jit-test/jit-test/tests/ion/dce-with-rinstructions.js:932:36 ReferenceError: Symbol is not defined (code 3, args "")
20:44:24     INFO -  INFO exit-status     : 3
20:44:24     INFO -  INFO timed-out       : False
20:44:24     INFO -  INFO stderr         2> /builds/slave/test/build/tests/jit-test/jit-test/tests/ion/dce-with-rinstructions.js:932:36 ReferenceError: Symbol is not defined
Fixed following tests:
  js/src/jit-test/tests/gc/bug-1053676.js
  js/src/jit-test/tests/ion/dce-with-rinstructions.js
  js/src/jsapi-tests/testForOfIterator.cpp

Green on try run:
  https://tbpl.mozilla.org/?tree=Try&rev=7acf018c08ae (Aurora Simulation)
  https://tbpl.mozilla.org/?tree=Try&rev=3f2210f83221 (trunk)
Attachment #8498895 - Flags: review?(nicolas.b.pierron)
Attachment #8498895 - Flags: review?(jorendorff)
Blocks: 1066432
Comment on attachment 8498895 [details] [diff] [review]
Do Symbol-related tests only if defined.

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

I kind of prefer the approach of using getBuildConfiguration [1,2], to reflect the configuration choices.  But I do not have a strong opinion here.

[1] http://dxr.mozilla.org/mozilla-central/search?q=path%3Ajs%2Fsrc%2Fjit-test+getBuildConfiguration&case=true&offset=101
[2] http://dxr.mozilla.org/mozilla-central/source/js/src/builtin/TestingFunctions.cpp?from=getBuildConfiguration#51

::: js/src/jit-test/tests/gc/bug-1053676.js
@@ +1,2 @@
>  // |jit-test| ion-eager;
> +if (typeof Symbol === "function") {

nit: It would be better to invert this condition, such as:

if (typeof Symbol != "function") quit(0);

and avoid the re-indentation.
Attachment #8498895 - Flags: review?(nicolas.b.pierron) → review+
Thank you for reviewing!

(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> Comment on attachment 8498895 [details] [diff] [review]
> Do Symbol-related tests only if defined.
> 
> Review of attachment 8498895 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I kind of prefer the approach of using getBuildConfiguration [1,2], to
> reflect the configuration choices.  But I do not have a strong opinion here.

I guess it might be better to replace all other usage of `typeof Symbol === "function"` tests at once, to make it easier to remove them after bug 1069416 is fixed.
So, I'd like to file another bug for it, okay?

> ::: js/src/jit-test/tests/gc/bug-1053676.js
> @@ +1,2 @@
> >  // |jit-test| ion-eager;
> > +if (typeof Symbol === "function") {
> 
> nit: It would be better to invert this condition, such as:
> 
> if (typeof Symbol != "function") quit(0);
> 
> and avoid the re-indentation.

Thanks, I'll fix it soon.
(In reply to Tooru Fujisawa [:arai] from comment #3)
> (In reply to Nicolas B. Pierron [:nbp] from comment #2)
> > I kind of prefer the approach of using getBuildConfiguration [1,2], to
> > reflect the configuration choices.  But I do not have a strong opinion here.
> 
> I guess it might be better to replace all other usage of `typeof Symbol ===
> "function"` tests at once, to make it easier to remove them after bug
> 1069416 is fixed.
> So, I'd like to file another bug for it, okay?

Sounds good to me, if Jason thinks that's good to modify getBuildConfiguration.
> if (typeof Symbol != "function") quit(0);
Updated :)
Attachment #8498895 - Attachment is obsolete: true
Attachment #8498895 - Flags: review?(jorendorff)
Attachment #8501383 - Flags: review?(jorendorff)
(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> (In reply to Tooru Fujisawa [:arai] from comment #3)
> > (In reply to Nicolas B. Pierron [:nbp] from comment #2)
> > > I kind of prefer the approach of using getBuildConfiguration [1,2], to
> > > reflect the configuration choices.  But I do not have a strong opinion here.
> > 
> > I guess it might be better to replace all other usage of `typeof Symbol ===
> > "function"` tests at once, to make it easier to remove them after bug
> > 1069416 is fixed.
> > So, I'd like to file another bug for it, okay?
> 
> Sounds good to me, if Jason thinks that's good to modify
> getBuildConfiguration.

Please don't do this.

The absence of Symbol is not expected to be a long-lived thing, only a short-term thing while symbol bugs are ironed out.  There's no reason to put lipstick on the pig.

Additionally, getBuildConfiguration is a SpiderMonkey shell-ism.  Tests that use it aren't immediately portable to run against other engines.  We should only write tests to depend on SpiderMonkey-specific behaviors when there's no other way to do it.  It's easy to do it another way, here.

With respect to quit(0), I would somewhat rather the test were encapsulated in a function, guarded like so:

if (typeof Symbol === "function")
  test();

because quit(0) is SpiderMonkey-specific, but this is enough of an existing problem I won't complain too strongly if you do the quit trick.
FYI, merge day is on Monday, so let's please get *something* landed here soon so I can verify a green Try run ahead of time.
Assignee: nobody → arai_a
[Tracking Requested - why for this release]:
Will cause perma-orange after the uplift to Aurora.
Thank you Waldo for letting me know about portability of testcase :)
About "quit", I'm not sure encapsulating the test in a function does not break the condition, so, if needed, I'd like to fix it later in another bug, together with other tests.

Green on try run for attachment 8501383 [details] [diff] [review]:
  https://tbpl.mozilla.org/?tree=Try&rev=528768f2e0a0 (trunk)
Aurora build fails :/ (bug 1079573)
  https://tbpl.mozilla.org/?tree=Try&rev=06dea6083065 (Aurora Simulation)
I'll test it after that bug is fixed.
But the difference between previous try run ( https://tbpl.mozilla.org/?tree=Try&rev=7acf018c08ae ) is only in testcase, so I guess it will pass unless other Symbol-related tests are added after it.

I'll ask jorendorff for reviewing later.
Adding needinfo? just in case.
Flags: needinfo?(jorendorff)
Attachment #8501823 - Flags: review?(bzbarsky)
Assignee: arai_a → jorendorff
Status: NEW → ASSIGNED
Comment on attachment 8501383 [details] [diff] [review]
Do Symbol-related tests only if defined.

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

Drop the changes to testForOfIterator.cpp. Otherwise this is good to land right now.
Attachment #8501383 - Flags: review?(jorendorff) → review+
Comment on attachment 8501823 [details] [diff] [review]
Fix some tests that fail when symbols are not defined

Wrong bug.
Attachment #8501823 - Attachment is obsolete: true
Attachment #8501823 - Flags: review?(bzbarsky)
Flags: needinfo?(jorendorff)
(In reply to Jason Orendorff [:jorendorff] from comment #11)
> Comment on attachment 8501383 [details] [diff] [review]
> Do Symbol-related tests only if defined.
> 
> Review of attachment 8501383 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Drop the changes to testForOfIterator.cpp. Otherwise this is good to land
> right now.

Thank you!

Try is running: https://tbpl.mozilla.org/?tree=Try&rev=cdc0efc72df6
Attachment #8501383 - Attachment is obsolete: true
Attachment #8501846 - Flags: review+
Try run was green :)
Keywords: checkin-needed
Sorry, I forgot about Aurora Simulation try.
removing checkin-needed for now.
Keywords: checkin-needed
Try run for Aurora Simulration started:
  https://tbpl.mozilla.org/?tree=Try&rev=e5e1b426677a
Almost passed Aurora Simulation Try run.
Remaining failure except M-e10s(bc1) will be fixed by bug 1079640.
About browser_e10s_switchbrowser.js in M-e10s(bc1), I'll investigate it, but I think it's not related to this patch.
Keywords: checkin-needed
M-e10s isn't support on !trunk. You're good :)
https://hg.mozilla.org/mozilla-central/rev/9bdba7a66515
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Status: RESOLVED → VERIFIED
Depends on: 1082202
You need to log in before you can comment on or make changes to this bug.