412247 - Allow to customize toBoolean conversion

Status: RESOLVED FIXED

(Rhino Graveyard :: Core, enhancement)

Description

[Marc]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.11) Gecko/20071204 Ubuntu/7.10 (gutsy) Firefox/2.0.0.11
Build Identifier: 

ECMA describes precisely that not null Object should be converted to true but this causes a problem when using Rhino to simulate Firefox behavior for document.all.

Rhino should give the possibility to configure if ECMA rules should be respected (default) or if Scriptable#getDefaultValue should be called on the object.

This is a critical requirement for us (HtmlUnit).

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Marc]

Attachment: patch adding Context.FEATURE_NON_ECMA_TO_BOOLEAN and related unit test

This patch adds the feature Context.FEATURE_NON_ECMA_TO_BOOLEAN providing the feature we need to customize boolean conversion.
It contains the appropriate unit test.

Comment 2

[Norris Boyd]

I posted to the mozilla.dev.tech.js-engine group and they referred me to bug 248549. There's a whole mechanism in SpiderMonkey for handling code that uses object detection on document.all that is deeper than just ToBoolean conversions. 

I'll think about how we approach this, but the right fix is more complex and probably should involve some special flag on host objects.

Comment 3

[Attila Szegedi]

Bug 248549 has a Calvin and Hobbes reference, yay!

Speaking seriously -- I couldn't fully understand the issue here as the bugreport escalated into an argument about principles and ideals. 

HtmlUnit people: would your proposed patch be sufficient to handle document.all, or do you envision you'll need other bits and pieces of support?

I'm willing to commit this patch, except I was thinking whether it would make more sense to introduce FEATURE_HTML_USER_AGENT as a generic flag to enable non-ECMA quirks found in HTML user agents instead of now adding FEATURE_NON_ECMA_TO_BOOLEAN, and then in future adding other flags for further quirks we discover down the road. Requests for other non-ECMA behaviour mostly reported via HtmlUnit people could also be implemented in such a way that they're enabled only when this feature is on (iteration order in for loop, unescaped / in character class in regex, etc.)

Comment 4

[Marc]

With the possibility to customize toBoolean I'll be able to handle "if (document.all)" and redefining the Boolean constructor I should be able to handle Boolean(document.all) without any change in Rhino itself.

I can't say if a general FEATURE_HTML_USER_AGENT feature would make sense or not. In the case of this issue, it requires additional work for Rhino user and the feature doesn't bring anything alone. If I correctly understand the comments concerning unescaped / in character class in regex, it should be made the default. Concerning the properties order, ECMA doesn't forbid to make it the default (and so does my patch in https://bugzilla.mozilla.org/show_bug.cgi?id=412928). At the end the boolean conversion is the only point that I currently see concerned by FEATURE_HTML_USER_AGENT.

Comment 5

[Norris Boyd]

I think a flag is the wrong approach for this problem. The issue is that you only want the special behavior for document.all, not all objects. Additionally, SpiderMonkey/Firefox evaluate (typeof document.all) -> "undefined", which your patch doesn't address. 

Instead, to match the SpiderMonkey/Firefox behavior we need a special host object flag that can indicate that the object needs to be treated specially. The object defined in your embedding to be returned by document.all would set this flag and get the special behavior. Other objects would behave normally. Marc, is your host object implementing document.all a subclass of ScriptableObject? I think the easiest way to have host objects define this flag would be to add a new boolean method on ScriptableObject with a default implementation you could override.

Comment 6

[Marc]

Seems that you've investigated it further than me: I didn't know (typeof document.all) -> "undefined".

Yes we implement ScriptableObject, a public or protected method on it to address current issue would be perfect for us.

Comment 7

[Norris Boyd]

Attachment: Patch implementing ScriptableObject.avoidObjectDectection

With proposed patch, and with a throwaway change for NativeDate.avoidObjectDetection to return true, Rhino
has the following behavior:

js> var obj = {d:new Date()};
js> obj.d ? true : false
false
js> if (obj.d) print(true); else print(false);
false
js> typeof obj.d
undefined
js> obj.d.getHours()
9

I'll test this and commit it unless someone objects.

Comment 8

[Marc]

This is fine for me.
(I have just checked and confirm that Boolean(x) - the missing part of the document.all problem - can be customized without any change within Rhino).

Comment 9

[Norris Boyd]

Checked in:

Checking in src/org/mozilla/javascript/ScriptRuntime.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/ScriptRuntime.java,v  <--
ScriptRuntime.java
new revision: 1.289; previous revision: 1.288
done
Checking in src/org/mozilla/javascript/ScriptableObject.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/ScriptableObject.java,v  <-
-  ScriptableObject.java
new revision: 1.126; previous revision: 1.125
done

Comment 10

[Norris Boyd]

(In reply to comment #8)
> This is fine for me.
> (I have just checked and confirm that Boolean(x) - the missing part of the
> document.all problem - can be customized without any change within Rhino).
> 

But you still need this change, right?

Comment 11

[Marc]

Yes, I need it too as I can't change it from "outside".

Thanks.

Comment 12

[VK]

> Specifically, the expression
>     document.all ? true : false
> results in false, but the expression
>     Boolean(document.all)
> results in true.

Right, this is what required by ECMAScript specs. Javascript
implements both primitive boolean value and Boolean object wrapper. In
logical operations objects are being downcasted first to primitives
with Boolean(false) resulting in true in many circumstances. Try this
for instance in any browser:

<script>
 var b1 = false;
 var b2 = new Boolean(false);

 /*1*/ window.alert(b1 ? 'true':'false'); // false
 /*2*/ window.alert(b2 ? 'true':'false'); // true
</script>

The moral is that you never ever use Boolean wrapper for Javascript
except one very particular situation which is not presented here.

In your case document.all returns undefined which is in Boolean
constructor context equal to Boolean(false) which is situation /*2*/
in the sample above.

So the options are either to break Rhino compatibility with ECMAScript
or to use the right style of boolean operations. I actually highly
surprised that on
https://bugzilla.mozilla.org/show_bug.cgi?id=412247
people seem ready to rather patch Rhino like no big problem which is
sorry to say but IMO a total nonsense.

It is especially strange because in Java itself there are primitive boolean false/true as well as Boolean wrapper.
Yet it is requested to break the standard-compliant behavior for emulated Firefox so it would be equal to the actual Firefox behavior in order to (OP quote) "simulate Firefox behavior for document.all".

Sorry guys, either I'm completely off the loop here or some participants are. I am ready to accept that I am.

Comment 13

[VK]

(In reply to comment #12)
> Yet it is requested to break the standard-compliant behavior for emulated
> Firefox so it would be equal to the actual Firefox behavior

Sorry:

"so it would be NOT equal"

Comment 14

[Norris Boyd]

(In reply to comment #12)
> So the options are either to break Rhino compatibility with ECMAScript
> or to use the right style of boolean operations. I actually highly
> surprised that on
> https://bugzilla.mozilla.org/show_bug.cgi?id=412247
> people seem ready to rather patch Rhino like no big problem which is
> sorry to say but IMO a total nonsense.

Actually, I see what we're doing here as a third option. We're breaking ECMAScript compatibility *only for host objects that explicitly request it*. So all current Rhino users should notice no change, and HtmlUnit can emulate Firefox behavior. You can argue that Firefox did the wrong thing in the way it implements document.all, but that ship has sailed and now HtmlUnit (and, by extension, Rhino) can best serve its users by emulating this behavior. Otherwise HtmlUnit is devalued as a testing tool.

Comment 15

[VK]

(In reply to comment #14)
> (In reply to comment #12)
> > So the options are either to break Rhino compatibility with ECMAScript
> > or to use the right style of boolean operations. I actually highly
> > surprised that on
> > https://bugzilla.mozilla.org/show_bug.cgi?id=412247
> > people seem ready to rather patch Rhino like no big problem which is
> > sorry to say but IMO a total nonsense.
> 
> Actually, I see what we're doing here as a third option. We're breaking
> ECMAScript compatibility *only for host objects that explicitly request it*. So
> all current Rhino users should notice no change, and HtmlUnit can emulate
> Firefox behavior. You can argue that Firefox did the wrong thing in the way it
> implements document.all, but that ship has sailed and now HtmlUnit (and, by
> extension, Rhino) can best serve its users by emulating this behavior.
> Otherwise HtmlUnit is devalued as a testing tool.

Can anyone explain me what is the "implementation" of document.all in Firefox and what is exactly wrong with it? On my Firefox 2.0.0.11 it does the same thing all browsers did before: reports undefined for missing DOM feature.

<script>
alert(document.all === undefined); // true
alert(document.FooBar === undefined); // true
</script>

I re-read the whole thread I see no bugs whatsoever - just OP that considers ECMAScript-compliant ternary expression as "a bug" so requesting to hack the engine. If there is not time to read ECMAScript 3rd ed. specs, please read my detailed post with explanations and samples at mozilla.dev.tech.js-engine: 
http://groups.google.com/group/mozilla.dev.tech.js-engine/msg/2884dfb8c781fa8d 
btw after knowing the real production algorithm one may realize that any DOM patches do not solve the OP's problem: one still has to break the JS engine as well (condition expression production in ternary).

Comment 16

[Marc]

You really seem to miss the point here. Just try following in your browser

javascript:void(alert(document.all.foo))

when an element has id="foo", you will get it and otherwise you will get undefined, and not Error: document.all has no properties.

Comment 17

[VK]

(In reply to comment #16)
> You really seem to miss the point here. Just try following in your browser
> 
> javascript:void(alert(document.all.foo))
> 
> when an element has id="foo", you will get it and otherwise you will get
> undefined, and not Error: document.all has no properties.

That is not what this bug is about, at least not as reported and discussed here or at http://groups.google.com/group/mozilla.dev.tech.js-engine/browse_frm/thread/69052055fba8f606

Maybe this is why it is so convoluted in both cases, because two groups or people are discussing two completely different issues at once.

[1]
This initial "bug" - which is not a bug at all - is about behavioral difference in Javascript between primitive boolean values and Boolean object wrapper, in the particular in the ternary operator.

var condition = Boolean(false);
condition ? doTrue() : doFalse();

will branch onto doTrue on any ECMAScript-compliant engine including Gecko. There is nothing to "fix" here whatsoever. If anyone gets surprised by the result then the only advise could be to read ECMAScript specs. If Boolean wrapper is really needed then a quick "guardian fix" could be in explicit downcasting into primitive before any boolean comparisons:

var condition = Boolean(false);
!condition ? doTrue() : doFalse();

now goes to doFalse "as expected"

[2]
The second issue is that in BackCompat (Quirk) mode Gecko partially emulates document.all.ElementID getter to meet some corporate customers ultimatum. This emulation is effective in BackCompat mode only but not in CSS1Compat mode.

<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta http-equiv="Content-Type"
 content="text/html; charset=iso-8859-1">
<title>Demo</title>
<script type="text/javascript">
function init() {
 window.alert('document mode: '+document.compatMode);
 window.alert(document.all); // undefined
 window.alert(document.all.Demo); // HTML element
}
window.onload = init;
</script>
</head>
<body>
<h1 id="Demo">Demo for BackCompat mode</h1>
</body>
</html>

at the same time

<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN">
<html>
<head>
<meta http-equiv="Content-Type"
 content="text/html; charset=iso-8859-1">
<title>Demo</title>
<script type="text/javascript">
function init() {
 window.alert('document mode: '+document.compatMode);
 window.alert(document.all === undefined); // true
 try {
  window.alert(document.all.Demo);
 }
 catch(e) {
  window.alert(e.message);
  // no such property exception
 }
}
window.onload = init;
</script>
</head>
<body>
<h1 id="Demo">Demo for CSS1Compat mode</h1>
</body>
</html>

This way it is indeed rather illogical picture: for BackCompat mode Gecko still reports document.all as undefined, yet document.all.ElementID is usable.

If someone doesn't like such behavior then one may file a bug like 
"document.all in BackCompat mode should reported as object"

I still missing any relevance of the issue [2] to the actual programming. The feature test is always must be done on the early stage and if "document.all" is reported as missing, why in name anyone will try to use it later so getting any troubles? That besides that problem as such is only exposed in quirk mode which is a very strange mode for Gecko: how anyone may make a compatible layout with quirk mode, so one client applying W3C model and other applying IE model?

An any case the bug title is misleading and 3/4 of the discussion is about the issue [1] above and not about [2]

Comment 18

[VK]

(In reply to comment #17)

> [2]
> The second issue is that in BackCompat (Quirk) mode Gecko partially emulates
> document.all.ElementID getter to meet some corporate customers ultimatum. This
> emulation is effective in BackCompat mode only but not in CSS1Compat mode.

By the way due to the same ultimating requirement Gecko in BackCompat emulating another IE feature: all ID'ed HTML elements being procreated as global Javascript variables.

<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta http-equiv="Content-Type"
 content="text/html; charset=iso-8859-1">
<title>Demo</title>
<script type="text/javascript">
function init() {
 window.alert(document.compatMode); // BackCompat
 window.alert(Demo); // HTMLElement
}
window.onload = init;
</script>
</head>
<body>
<h1 id="Demo">Demo for BackCompat mode</h1>
</body>
</html>

Is it planned to patch toBoolean for it as well?

Comment 19

[Norris Boyd]

(In reply to comment #18)
> [snip]
> 
> Is it planned to patch toBoolean for it as well?
> 

No.

Comment 20

[Norris Boyd]

Complete fix for bug 412247 - Allow to customize toBoolean conversion
Fix misspelling in method name (!) and correct support for Boolean(expr).

Checking in src/org/mozilla/javascript/NativeBoolean.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/NativeBoolean.java,v  <-- NativeBoolean.java
new revision: 1.34; previous revision: 1.33
done
Checking in src/org/mozilla/javascript/ScriptRuntime.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/ScriptRuntime.java,v  <-- ScriptRuntime.java
new revision: 1.290; previous revision: 1.289
done
Checking in src/org/mozilla/javascript/ScriptableObject.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/ScriptableObject.java,v  <--  ScriptableObject.java
new revision: 1.127; previous revision: 1.126
done