1264085 - shell/warning.js is going to permafail when Gecko 48 merges to Beta

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]: SM permafail when Gecko 48 merges to Beta.

Any idea what might have changed in RELEASE_BUILD land recently here, Arai? This is burning all SM jobs.

https://treeherder.mozilla.org/logviewer.html#?job_id=19227326&repo=try

## shell/warning.js: rc = 3, run time = 0.062321
1170716: Add js shell functions to get last warning
shell/warning.js:16:1 Error: Assertion failed: got false, expected true
Stack:
  @shell/warning.js:16:1
TEST-UNEXPECTED-FAIL | shell/warning.js | (args: "")

Comment 1

[Tooru Fujisawa [:arai]]

This is a regression from bug 1049041.

The warning in String#contains is non-release only.
We should use another warning that is enabled on all branches.

Comment 2

[Tooru Fujisawa [:arai]]

the code is changed again in bug 1103588, to expression closure.
  https://hg.mozilla.org/integration/mozilla-inbound/rev/6891e4414ffaa50352ffdbab21997a6c431e0cae#l2.14

> -let line0 = new Error().lineNumber;
> -assertEq("foo".contains("bar"), false);
> +eval(`(function() "This is an expression closure.")`);

That is also non-release only warning...

> template <typename ParseHandler>
> bool
> Parser<ParseHandler>::warnOnceAboutExprClosure()
> {
> #ifndef RELEASE_BUILD
>     JSContext* cx = context->maybeJSContext();
>     if (!cx)
>         return true;
> 
>     if (!cx->compartment()->warnedAboutExprClosure) {
>         if (!report(ParseWarning, false, null(), JSMSG_DEPRECATED_EXPR_CLOSURE))
>             return false;
>         cx->compartment()->warnedAboutExprClosure = true;
>     }
> #endif
>     return true;
> }

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Use not-non-release-only warning in js/src/tests/shell/warning.js.

Following warning is enabled on all branches, right?
Will it be removed or disabled near future?

> MSG_DEF(JSMSG_DEPRECATED_BLOCK_SCOPE_FUN_REDECL, 1, JSEXN_NONE, "redeclaration of block-scoped function `{0}' is deprecated")

Comment 4

[Jason Orendorff [:jorendorff]]

(In reply to Tooru Fujisawa [:arai] from comment #3)
> Following warning is enabled on all branches, right?
> Will it be removed or disabled near future?

Looks like it.

In any case fixing the build before merge is the main thing...

Comment 5

[Jason Orendorff [:jorendorff]]

(In reply to Jason Orendorff [:jorendorff] from comment #4)
> (In reply to Tooru Fujisawa [:arai] from comment #3)
> > Following warning is enabled on all branches, right?
> > Will it be removed or disabled near future?
> 
> Looks like it.

I meant to say, yes, this warning appears to be enabled on all branches.

It won't be removed in the near future. Deprecation is always a long play...

Comment 6

[Shu-yu Guo [:shu]]

Comment on attachment 8740831 [details] [diff] [review]
Use not-non-release-only warning in js/src/tests/shell/warning.js.

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

::: js/src/tests/shell/warning.js
@@ +8,5 @@
>  // Warning with JSEXN_NONE.
>  
>  enableLastWarning();
>  
> +eval(`{ function f() {} function f() {} }`);

So the goal here is to do *something* that generates a warning? Because this warning around functions-in-block will go away too eventually. Is there a more permanent warning we can use?

Comment 7

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)

The testcase is to test getLastWarning shell builtin on JSEXN_NONE and one more non-JSEXN_NONE type.  I wanted to test both types, because they use different path for warning.name property:

>     if (report->exnType == JSEXN_NONE)
>         nameStr = JS_NewStringCopyZ(cx, "None");
>     else
>         nameStr = GetErrorTypeName(cx->runtime(), report->exnType);

So, yes, I want to do something that generates warning messages with JSEXN_NONE there.


Then, currently there are following warning message that uses JSEXN_NONE but not related to deprecation.

> MSG_DEF(JSMSG_ALREADY_HAS_PRAGMA,      2, JSEXN_NONE, "{0} is being assigned a {1}, but already has one")
> MSG_DEF(JSMSG_STMT_AFTER_RETURN,       0, JSEXN_NONE, "unreachable code after return statement")
> MSG_DEF(JSMSG_USE_ASM_TYPE_OK,         1, JSEXN_NONE,    "Successfully compiled asm.js code ({0})")

I have no idea how to hit JSMSG_ALREADY_HAS_PRAGMA in shell.

JSMSG_STMT_AFTER_RETURN was the reason why getLastWarning was added (bug 1170716), so I couldn't use it (because getLastWarning was added before JSMSG_STMT_AFTER_RETURN).  Maybe I could change the testcase to use JSMSG_STMT_AFTER_RETURN instead, but it might be better to use other warning message (also just for coverage).

JSMSG_USE_ASM_TYPE_OK could also be used, but it has no information about line/column...

Comment 8

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/18ba8acba7902115a6f897d7d22b1cc33cb56008
Bug 1264085 - Use not-non-release-only warning in js/src/tests/shell/warning.js. r=shu

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/18ba8acba790