1127900 - Scriptable XPIDL interfaces should not define flags >= 0x8000 0000 because they break JS binary ops

Status: NEW

(Core :: General, defect)

Description

[:Gijs (he/him)]

This is not fun:

var f = 0x80000000;
(f & f) == f // evaluates to false in JS


The reason for this is that JS binary ops convert to *signed* 32-bit values, and so anything (IIUC) using flags up to 0x80000000 is OK, but 0x80000000 itself and over is not.

This bites at least (from a naive MXR search):

* CHROME_OPENAS_CHROME on nsIWebBrowserChrome,
* kInitSaveDownloadFonts and kInitSaveAll on nsIPrintSettings,
* DELETE_ON_CLOSE on nsIFile;

nsIWebBrowserChrome has some constant space around 0x00N00000, I think, but nsIFile passes to NSPR and nsIPrintSettings seems to completely use the 32-bit flag space here.

Equally, I'd imagine that "fixing" JS binary ops to 'just' use unsigned numbers isn't feasible because web compat. :-\

Are there other things we can do here, besides just hoping all the JS consumers will remember to use other hackarounds to figure out if flags are included?

Comment 1

[Boris Zbarsky [:bzbarsky]]

So more precisely, for flags that are _read_ from JS you don't want to use 0x80000000 or above.  There's no problem with passing in 0x80000000 from JS as a flag.

So for example, the nsIFile case seems ok to me at first glance, unless some JS code is trying to check whether callers want this flag and then pass it down or not or some such?

Similar for printsettings: the flags are only used for nsIPrintSettingsService.initPrintSettingsFromPrefs

And same for nsIWebBrowserChrome.

> Equally, I'd imagine that "fixing" JS binary ops to 'just' use unsigned numbers isn't
> feasible because web compat. :-\

And the fact that it would change the JS language in general, etc, sure.

> Are there other things we can do here, besides just hoping all the JS consumers will
> remember to use other hackarounds to figure out if flags are included?

I'd really like to understand the actual use case here so I can answer this question.

For example, when testing for a particular flag one would normally do "if (flags & flag)" which works fine with the current JS setup.  You're right that you run into trouble when doing equality compares on the return value of a bitwise op, but when would one do that in JS?

Comment 2

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #1)
> So more precisely, for flags that are _read_ from JS you don't want to use
> 0x80000000 or above.  There's no problem with passing in 0x80000000 from JS
> as a flag.
> 
> So for example, the nsIFile case seems ok to me at first glance, unless some
> JS code is trying to check whether callers want this flag and then pass it
> down or not or some such?
> 
> Similar for printsettings: the flags are only used for
> nsIPrintSettingsService.initPrintSettingsFromPrefs
> 
> And same for nsIWebBrowserChrome.
> 
> > Equally, I'd imagine that "fixing" JS binary ops to 'just' use unsigned numbers isn't
> > feasible because web compat. :-\
> 
> And the fact that it would change the JS language in general, etc, sure.
> 
> > Are there other things we can do here, besides just hoping all the JS consumers will
> > remember to use other hackarounds to figure out if flags are included?
> 
> I'd really like to understand the actual use case here so I can answer this
> question.
> 
> For example, when testing for a particular flag one would normally do "if
> (flags & flag)" which works fine with the current JS setup.  You're right
> that you run into trouble when doing equality compares on the return value
> of a bitwise op, but when would one do that in JS?

So I ran into this when working around the lack of window.opener in tests affected by my patch for bug 1115333. I tried to read chrome flags on a window and checked that it specified this flag (and some others) a la the test here: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_openDialog.js#136

The reason I actually checked for the flag result can be chalked up to paranoia and the fact that if checking against multiple flags, like here: https://bugzilla.mozilla.org/show_bug.cgi?id=1048618#c8 you do need the stricter check, ie:

var needFlags = flag1 | flag2;

if (foo & needFlags) { /* this will run even if only flag1 or flag2 matches */ }

Comment 3

[Boris Zbarsky [:bzbarsky]]

> I tried to read chrome flags on a window

Aha!  So this is in fact a problem with nsIWebBrowserChrome.  The other two things don't seem to have the flags readable, right?

So yes, if checking for multiple flags you run into a problem unless you remember to throw in a >>>0 on the one side or a |0 on the other side....