572411 - removal of JSObjectOps::checkAccess

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

Currently only js_WithClass provides a custom checkAccess. This suggests to remove the corresponding method and just do the check for a With objects in js_CheckMethod. This will replace an indirect call with highly predictable if check.

Similarly only With and XML classes provide a custom implementation of JSObjectOps::defaultValue. Moreover, js_DefaultValue just tries to call various methods on the object and the With class routes the methods to its proto object. So even if a With object escapes to a script using, for example, a debugger or other native extension to the engine (there are no other ways to get an access for the With object from a script), js_DefaultValue would do the same job for With as for With.__proto__. 

So if defaultValue is eliminated, js_DefaultValue would need to check only for the XML case and only for non-string hints as only for those xml_defaultValue diverges from js_DefaultValue.

Comment 1

[Igor Bukanov]

Attachment: patch

The patch is a straightforward implementation of the comment 0. The only issue is that I changed TraceRecorder::guardNativeConversion to guard on the class in the non-native case. Currently the code just guard that ops->defaultValue is js_DefaultValue. But this is insufficient as it should also guard that JSClass::convert is a stub one.

Comment 2

[Brendan Eich [:brendan]]

Separate this into two bugs, one for checkAccess and maybe one (we should discuss design first) for defaultValue.

ECMA-262 all editions has a [[DefaultValue]] and until we decide to get rid of JSObjectOps altogether and use if statements (and only a few, or really, one if testing isProxy()), we should not piece-wise eviscerate JSObjectOps.

/be

Comment 3

[Igor Bukanov]

(In reply to comment #2)
> ECMA-262 all editions has a [[DefaultValue]] and until we decide to get rid of
> JSObjectOps altogether and use if statements (and only a few, or really, one if
> testing isProxy()), we should not piece-wise eviscerate JSObjectOps.

The proxy uses the standard js_DefaultValue implementation. Is it an oversight? Also I have realized that js_DefaultValue does not need to treat XML in any special way. Thus currently defaultHook just adds a useless level of indirection.

Besides JSClass::convert is not specified by ECMA yet it exists in JSClass and together with a possubility to overwrite toString implementation in JSObjectOps::getProperty provides sufficient customization possibilities to overwrite the default behavior.

Comment 4

[Igor Bukanov]

Attachment: v2

The patch just removes JSObjectOps::checkAccess leaving defaultValue for another bug.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 451680 [details] [diff] [review]
v2

We prefer js::CheckAccess these days, IIRC. I've used that instead of js_ (see bug 547297 for the full work item, but we have worked "ahead" where possible; at least, I have).

Remove JSCheckAccessIdOp typedef and comment too?

Really, you want mrbkap to r+ this.

/be

Comment 6

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/a765e646ab46 - pushed with js::CheckAccess and with JSCheckAccessIdOp/comments removed.

Comment 7

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/a765e646ab46