Closed
Bug 253150
Opened 20 years ago
Closed 20 years ago
JSRESOLVE_DETECTING tweaks
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
mozilla1.8beta1
People
(Reporter: brendan, Assigned: brendan)
Details
(Keywords: fixed-aviary1.0, fixed1.7, js1.5)
Attachments
(2 files, 1 obsolete file)
9.22 KB,
patch
|
shaver
:
review+
jst
:
superreview+
brendan
:
approval-aviary+
|
Details | Diff | Splinter Review |
231.77 KB,
application/zip
|
Details |
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•20 years ago
|
||
Will you be able to make (document.all == null) == true and (document.all != null) == false as well? ditto undefined.
Assignee | ||
Comment 2•20 years ago
|
||
Patch after I get lunch. /be
Status: NEW → ASSIGNED
Flags: blocking-aviary1.0PR?
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta
Assignee | ||
Comment 3•20 years ago
|
||
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•20 years ago
|
||
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•20 years ago
|
||
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•20 years ago
|
||
I don't have any data on the usage of strict equality operators, but feel it is pretty uncommon.
Assignee | ||
Comment 7•20 years ago
|
||
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
Assignee | ||
Updated•20 years ago
|
Attachment #154430 -
Flags: superreview?(jst)
Attachment #154430 -
Flags: review?(shaver)
Assignee | ||
Comment 8•20 years ago
|
||
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
Attachment #154430 -
Attachment is obsolete: true
Attachment #154430 -
Flags: superreview?(jst)
Attachment #154430 -
Flags: review?(shaver)
Assignee | ||
Comment 9•20 years ago
|
||
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
Assignee | ||
Comment 10•20 years ago
|
||
Comment on attachment 154528 [details] [diff] [review] better patch Bob is gonna test this patch, but feel free to review starting now :-). /be
Attachment #154528 -
Flags: superreview?(jst)
Attachment #154528 -
Flags: review?(shaver)
Comment 11•20 years ago
|
||
Comment on attachment 154528 [details] [diff] [review] better patch sr=jst
Attachment #154528 -
Flags: superreview?(jst) → superreview+
Comment 12•20 years ago
|
||
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•20 years ago
|
||
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•20 years ago
|
||
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.
Attachment #154528 -
Flags: review?(shaver) → review+
Assignee | ||
Comment 15•20 years ago
|
||
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
Attachment #154528 -
Flags: approval-aviary+
Assignee | ||
Comment 16•20 years ago
|
||
.
Updated•20 years ago
|
Flags: blocking-aviary1.0PR?
Comment 17•20 years ago
|
||
This patch doesn't apply to 1.7.5. Is this something we want for 1.7?
Assignee | ||
Comment 18•20 years ago
|
||
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
Keywords: fixed1.7
Updated•19 years ago
|
Flags: testcase?
Comment 19•19 years ago
|
||
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-253150.js,v <-- regress-253150.js
Flags: testcase? → testcase+
You need to log in
before you can comment on or make changes to this bug.
Description
•