1284099 - Assert nsIScriptError.*Flags constants match to JSREPORT_*

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

There are 2 places that defines flag constants.

https://dxr.mozilla.org/mozilla-central/rev/39dffbba764210b25bfc1e749b4f16db77fa0d46/js/xpconnect/idl/nsIScriptError.idl#22
>     /** pseudo-flag for default case */
>     const unsigned long errorFlag = 0x0;
> 
>     /** message is warning */
>     const unsigned long warningFlag = 0x1;
> 
>     /** exception was thrown for this case - exception-aware hosts can ignore */
>     const unsigned long exceptionFlag = 0x2;
> 
>     // XXX check how strict is implemented these days.
>     /** error or warning is due to strict option */
>     const unsigned long strictFlag = 0x4;


https://dxr.mozilla.org/mozilla-central/rev/39dffbba764210b25bfc1e749b4f16db77fa0d46/js/src/jsapi.h#5190
> #define JSREPORT_ERROR      0x0     /* pseudo-flag for default case */
> #define JSREPORT_WARNING    0x1     /* reported via JS_ReportWarning */
> #define JSREPORT_EXCEPTION  0x2     /* exception was thrown */
> #define JSREPORT_STRICT     0x4     /* error or warning due to strict option */


We should at lease assert they're same.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Assert nsIScriptError.*Flags constants match to JSREPORT_*.

Added assertion for flags.

I'm not sure how to handle nsIScriptError.infoFlag there, as it's used only outside of SpiderMonkey.
Maybe we could define JSREPORT_USER or something in jsapi.h to avoid future conflict?

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 8767528 [details] [diff] [review]
Assert nsIScriptError.*Flags constants match to JSREPORT_*.

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

Looks good to me, but forwarding review to Ben Turner, who owns this code.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8767528 [details] [diff] [review]
Assert nsIScriptError.*Flags constants match to JSREPORT_*.

Andrea, would you mind reviewing this?

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 8767528 [details] [diff] [review]
Assert nsIScriptError.*Flags constants match to JSREPORT_*.

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

::: dom/workers/WorkerPrivate.cpp
@@ +5920,5 @@
> +  static_assert(nsIScriptError::errorFlag == JSREPORT_ERROR &&
> +                nsIScriptError::warningFlag == JSREPORT_WARNING &&
> +                nsIScriptError::exceptionFlag == JSREPORT_EXCEPTION &&
> +                nsIScriptError::strictFlag == JSREPORT_STRICT,
> +                "flags should be consistent");

This static_assert should be moved into ./js/xpconnect/src/nsScriptError.cpp
Plus you should add nsIScriptError::infoFlag == JSREPORT_STRICT_MODE_ERROR

Comment 5

[Tooru Fujisawa [:arai]]

Thank you all :)

(In reply to Andrea Marchesini (:baku) from comment #4)
> Plus you should add nsIScriptError::infoFlag == JSREPORT_STRICT_MODE_ERROR

I don't think they have same meaning. Those 2 constants are just conflicting.
There is no place that nsIScriptError::infoFlag flows into the code that checks JSREPORT_STRICT_MODE_ERROR,
and I'm about to remove JSREPORT_STRICT_MODE_ERROR constant in bug 1283058 because it's not used anywhere.

Maybe we'd better adding another constant in jsapi.h for user-defined flag(s).
will prepare a patch for that.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Add JSREPORT_USER_1 for user-defined flag in JSErrorReport.flags.

This patch is based on patches in bug 1283058, that removes unused and conflicting JSREPORT_STRICT_MODE_ERROR (0x08).

As long as the flag nsIScriptError::infoFlag (0x08) is used only outside of SpiderMonkey, I think we should define it as user-defined flag, to avoid future conflict.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Assert nsIScriptError.*Flags constants match to JSREPORT_*.

Moved js/xpconnect/src/nsScriptError.cpp and added `nsIScriptError::infoFlag == JSREPORT_USER_1`.

Comment 8

[Andrea Marchesini [:baku]]

> There is no place that nsIScriptError::infoFlag flows into the code that
> checks JSREPORT_STRICT_MODE_ERROR,
> and I'm about to remove JSREPORT_STRICT_MODE_ERROR constant in bug 1283058
> because it's not used anywhere.

Thanks for doing this!

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 8770351 [details] [diff] [review]
Part 1: Add JSREPORT_USER_1 for user-defined flag in JSErrorReport.flags.

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

::: js/src/jsapi.h
@@ +5189,5 @@
>  #define JSREPORT_WARNING    0x1     /* reported via JS_ReportWarning */
>  #define JSREPORT_EXCEPTION  0x2     /* exception was thrown */
>  #define JSREPORT_STRICT     0x4     /* error or warning due to strict option */
>  
> +#define JSREPORT_USER_1     0x8     /* user-defined flag */

Guh, this is awful.  Let's try to make this thing temporary, hopefully?

Comment 10

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)

So, to make this temporary, we could do either following:
  1. Make it JSREPORT_INFO and support it in SpiderMonkey side (not sure how it works in SpiderMonkey tho..)
  2. Stop using nsIScriptError::infoFlag and store it in different member

Will check how nsIScriptError::infoFlag is used

Comment 11

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/50f6751e3ef22450b0ea457363343d95a331b09f
Bug 1284099 - Part 1: Add JSREPORT_USER_1 for user-defined flag in JSErrorReport.flags. r=jwalden

https://hg.mozilla.org/integration/mozilla-inbound/rev/50c2b9e5f491bc89186235e52c6c95a1d495df11
Bug 1284099 - Part 2: Assert nsIScriptError.*Flags constants match to JSREPORT_*. r=baku

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/50f6751e3ef2
https://hg.mozilla.org/mozilla-central/rev/50c2b9e5f491

Comment 13

[Jeff Walden [:Waldo]]

(In reply to Tooru Fujisawa [:arai] from comment #10)
> So, to make this temporary, we could do either following:
>   1. Make it JSREPORT_INFO and support it in SpiderMonkey side (not sure how
> it works in SpiderMonkey tho..)
>   2. Stop using nsIScriptError::infoFlag and store it in different member

Longer-term, I'm thinking more about replacing the entire reporting mechanism with something better, that's more focused on creating and throwing exceptions.  And that doesn't have this JSREPORT_* thing at all, because the language itself has nothing like it.

Shorter-term, I dunno, maybe there's *some* better way to do things here.  But I didn't look closely.  This is more of an aspirational complaint/request, than one looking for a definite, concrete, near-term result.