253150 - JSRESOLVE_DETECTING tweaks

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

1.  JSOP_TYPEOF should be considered to be "detecting".
2.  The strict warning "reference to undefined property foo" given for o.foo
should not be given for detecting cases.

See bug 248549 comment 50.

/be

Comment 1

[Bob Clary [:bc] (inactive)]

Will you be able to make (document.all == null) == true and (document.all !=
null) == false as well? ditto undefined.

Comment 2

[Brendan Eich [:brendan]]

Patch after I get lunch.

/be

Comment 3

[Brendan Eich [:brendan]]

bc: I'll try -- no promises.  Do you actually see such tests in pages?  How
many, out of total that detect document.all?

/be

Comment 4

[Bob Clary [:bc] (inactive)]

I noticed this over the weekend in <http://ori.dhhs.gov/> in an old Macromedia
Dreamweaver function MM_showHideLayers() v 2.0. Note the page also contains a
commented out newer version 3.0 of the same function which does not use the
document.all != null test. We actually work with the v 2.0 script because of the
document.all != null test. Even though I don't have a list of sites which are
broken because of this, it still worries me a bit since these cases could result
in our going down the IE path in inappropriate circumstances. 

If we can't fix it, I would really like to hear from Macromedia/Adobe/Oracle/IBM
etc to make sure this doesn't cause them problems.

Looking in google:

"document.all != null" OR "document.all == null" 
shows a "few" (642 English pages/1,060 Total) pages, 

"document.all != undefined"  OR "document.all == undefined" also is small (132
English/139 total) 

"typeof(document.all) == 'undefined'"  OR "typeof(document.all) == 'undefined'"
is smaller still (123 English/130 Total).

These are tiny relative to the "massive" (506,000 English/607,000 Total) pages
returned for "document.all", but could include important edge cases not easily
found using the approaches I have been using.

So, my recommendation is if we can make this truly "undetecting" we should.

Comment 5

[Nicholas Allen]

Does anyone use document.all === undefined or document.all !== undefined?  I'd
imagine the number would be similarly small to some of these other cases.

Comment 6

[Bob Clary [:bc] (inactive)]

I don't have any data on the usage of strict equality operators, but feel it is
pretty uncommon.

Comment 7

[Brendan Eich [:brendan]]

Attachment: proposed patch

This handles document.all == null and document.all != null, but not undefined,
and not the strict equality ops.

Handling undefined would take more work, because you have to look up undefined
at runtime after recognizing JSOP_NAME <undefined> in the bytecode, in lieu of
JSOP_NULL.  The undefined property of the global object was not added till
ECMA-262 Edition 3, and it's read/write, so script can redefine it.

This patch adds 76 bytes of -O2 Makefile.ref text size to jsobj.o with
gcc3.3.2.

/be

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 154430 [details] [diff] [review]
proposed patch

Better patch, this is not extensive enough, and worse, the JSOP_NULL;
JSOP_EQ|JSOP_NE test is busted!

/be

Comment 9

[Brendan Eich [:brendan]]

Attachment: better patch

This handles document.all tests againts null using == and !=, and against
undefined using ==, !=, ===, !===.  It does not worry about JavaScript1.2.  It
does not worry about the possibility that someone might redefine undefined.  It
doesn't worry about tests against void 0 or void (any expression).

You can play with it in the js shell using the built-in "it" object, by setting
it.noisy to true:

js> it.noisy=true
true
js> it.all == null
resolving its property all, flags {qualified,,detecting}
getting its property all, current value undefined
true
js> it.all != null
resolving its property all, flags {qualified,,detecting}
getting its property all, current value undefined
false
js> it.all == undefined
resolving its property all, flags {qualified,,detecting}
getting its property all, current value undefined
true
js> it.all != undefined
resolving its property all, flags {qualified,,detecting}
getting its property all, current value undefined
false
js> it.all === undefined
resolving its property all, flags {qualified,,detecting}
getting its property all, current value undefined
true
js> it.all !== undefined
resolving its property all, flags {qualified,,detecting}
getting its property all, current value undefined
false
js> it.all !== void 0
resolving its property all, flags {qualified,,}
getting its property all, current value undefined
false

/be

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 154528 [details] [diff] [review]
better patch

Bob is gonna test this patch, but feel free to review starting now :-).

/be

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 154528 [details] [diff] [review]
better patch

sr=jst

Comment 12

[Bob Clary [:bc] (inactive)]

Attachment: zipped test results

The zip file contains the full results. Here is the summary.

Spidered home pages of test sites list from bug 248549 
combined with top 70 sites using Spider 0.0.0.7
from http://bclary.com/2004/07/10/mozilla-spiders
with user hooks http://bclary.com/2004/07/10/tests/combinedhooks.js
which outputs Quirks/Standards mode, OBJECT/EMBED information and
injects mouse[over|out] pairs on each element.

CSS Parsing Errors and JavaScript Strict Warnings, and
JavaScript Chrome Errors were enabled.

628 pages attempted
-73 failed 
===
555 tested

Quirks vs. Standards Mode
=========================
505 pages in Quirks mode


undetected document.all
=======================

23 occurences of Non-standard document.all in 22 pages.
All occur in pages in Quirks mode.

 2 JavaScript Exceptions involving document.all

  JavaScript Exception: document.all.namedItem is not a function. 
  Source File: http://games.jolt.co.uk/, Line: 166.
  
  JavaScript Exception: document.all.plazadiv[kkk] has no properties. 
  Source File: http://ohmynews.com/, Line: 2845.

http://archavoda.org.il/
  partly fixed. mixed object detection.

http://bajajcapital.com/
  not fixed. nn4/ie4 detection.

http://clicpeugeot.com/
  mostly works, detects nn4 then assumes ie4. minor global id var issue

http://coe.int/
  fixed.

http://dell.com/
  worked already, coded to be cross browser though uses some MSIE only 
  features which are detected. document.all non-standard message may be 
  spurious.

http://dom.com/
  not fixed, uses HTMLElement.parentElement

http://dramaten.se/
  fixed. site is ie5+/gecko using XMLHttpRequest etc. The document.all error
  was just an oversight.

http://egged.co.il/
  not fixed. HTMLElement.parentElement.

http://emhart-vic.com/
  not fixed due to nn4/ie4 detection.

http://games.jolt.co.uk/
  partly fixed. document.all.namedItem,
  
http://haaretzdaily.com/
  partly fixed, nn4/ie4 detection, window.event

The remaining sites I leave as an exercise for the reader.

http://iafdb.travel.state.gov/
http://imbc.com/
http://janusys.com/
http://jockey.com/
http://joins.com/
http://ohmynews.com/
http://orange.co.il/
http://pandasoftware.com/
http://timeoutny.com/
http://tvb.com/
http://walla.co.il/

Errors
======

22551 JavaScript Warnings of which 10834 are unique
 3078 JavaScript Exceptions of which 194 are unique
   40 CSS Warnings of which 33 are unique
 8295 CSS Errors of which 2645 are unique

See bug253150.log for details and error-summary.txt for a summary.

Plugins and ActiveX
===================

14 WARNING: OBJECT Tag has FLASH WMODE but child EMBED Tag does not.
11 WARNING: OBJECT Tag does not contain EMBED Tag
 1 WARNING: SCRIPT FOR EVENT

  Unique OBJECT CLSID/DATA  
clsid:d27cdb6e-ae6d-11cf-96b8-444553540000 occurred 135 times.
clsid:fdc7a535-4070-4b92-a0ea-d9994bcc0dc5 occurred 2 times.
clsid:aaf15a90-f3ec-4fee-9a00-f65b25b83d05 occurred 2 times.
clsid:6bf52a52-394a-11d3-b153-00c04f79faa6 occurred 1 times.
clsid:575a1237-563b-11d4-9105-00105aa4854f occurred 1 times.

d27cdb6e-ae6d-11cf-96b8-444553540000 Flash
fdc7a535-4070-4b92-a0ea-d9994bcc0dc5 RealOnePlayer
aaf15a90-f3ec-4fee-9a00-f65b25b83d05 ESPN DIGStreamClientInfo
6bf52a52-394a-11d3-b153-00c04f79faa6 Windows Media Player 7+
575a1237-563b-11d4-9105-00105aa4854f Ready4Call.com

Comment 13

[Bernard Alleysson]

Just for the record, here is a site that uses "typeof" to detect IE (main menu
is now broken): 
http://www.activeplus.com
I expect it to be fixed along with this bug.

Comment 14

[Mike Shaver (:shaver emeritus)]

Comment on attachment 154528 [details] [diff] [review]
better patch

I'd like a few comments in Detecting describing the script that would
correspond to the different cases, but r=shaver anyway because I'm a soft
touch.

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 154528 [details] [diff] [review]
better patch

Shaver, your wish is my command.  Check out these comments:

/*
 * Given pc pointing after a property accessing bytecode, return true if the
 * access is a "object-detecting" in the sense used by web pages, e.g., when
 * checking whether document.all is defined.
 */
static JSBool
Detecting(JSContext *cx, jsbytecode *pc)
{
    JSScript *script;
    jsbytecode *endpc;
    JSOp op;
    JSAtom *atom;

    script = cx->fp->script;
    for (endpc = script->code + script->length; pc < endpc; pc++) {
	/* General case: a branch or equality op follows the access. */
	op = (JSOp) *pc;
	if (js_CodeSpec[op].format & JOF_DETECTING)
	    return JS_TRUE;

	/*
	 * Special case #1: handle (document.all == null).  Don't sweat about
	 * JS1.2's revision of the equality operators here.
	 */
	if (op == JSOP_NULL) {
	    if (++pc < endpc)
		return *pc == JSOP_EQ || *pc == JSOP_NE;
	    break;
	}

	/*
	 * Special case #2: handle (document.all == undefined).  Don't worry
	 * about someone redefining undefined, which was added by Edition 3,
	 * so was read/write for backward compatibility.
	 */
	if (op == JSOP_NAME) {
	    atom = GET_ATOM(cx, script, pc);
	    if (atom == cx->runtime->atomState.typeAtoms[JSTYPE_VOID] &&
		(pc += js_CodeSpec[op].length) < endpc) {
		op = (JSOp) *pc;
		return op == JSOP_EQ || op == JSOP_NE ||
		       op == JSOP_NEW_EQ || op == JSOP_NEW_NE;
	    }
	    break;
	}

	/* At this point, anything but grouping means we're not detecting. */
	if (op != JSOP_GROUP)
	    break;
    }
    return JS_FALSE;
}

Ben said ok for aviary branch.	Fixed there and on trunk.

/be

Comment 16

[Brendan Eich [:brendan]]

.

Comment 17

[Mike Kaply [:mkaply]]

This patch doesn't apply to 1.7.5.

Is this something we want for 1.7?

Comment 18

[Brendan Eich [:brendan]]

Sorry, forgot to mention and add the keyword -- this is all on the 1.7 branch. 
All the aviary JS engine changes were made to the 1.7 branch at the same time.

$ cvs diff -rAVIARY_1_0_20040515_BRANCH -rMOZILLA_1_7_BRANCH jsobj.c
$ cvs diff -rAVIARY_1_0_20040515_BRANCH -rMOZILLA_1_7_BRANCH jsopcode.tbl
$

/be

Comment 19

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-253150.js,v  <--  regress-253150.js

Comment 20

[Bob Clary [:bc] (inactive)]

verified fixed.