Last Comment Bug 256932 - Expose elements by their id/name in the global scope (quirks mode only, for IE compat) (GlobalScopePolluter)
: Expose elements by their id/name in the global scope (quirks mode only, for I...
Status: RESOLVED FIXED
: fixed-aviary1.0, fixed1.7.5
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: general
: Hixie (not reading bugmail)
Mentors:
: 271424 (view as bug list)
Depends on:
Blocks: 255514 257602 257649
  Show dependency treegraph
 
Reported: 2004-08-25 18:12 PDT by Johnny Stenback (:jst, jst@mozilla.com)
Modified: 2007-01-06 23:32 PST (History)
20 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Expose elements by their name/id in the global scope. (22.26 KB, patch)
2004-08-25 18:30 PDT, Johnny Stenback (:jst, jst@mozilla.com)
brendan: review+
brendan: superreview+
brendan: approval‑aviary+
asa: approval1.7.5+
Details | Diff | Splinter Review
diff -w (19.70 KB, patch)
2004-08-25 19:11 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
tests (7.01 KB, application/zip)
2004-08-25 20:16 PDT, Bob Clary [:bc:]
no flags Details
Only for non-qualified resolves, and warn when these references are used. (28.28 KB, patch)
2004-08-25 20:35 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
patch: add message to dom.properties (885 bytes, text/plain)
2004-08-26 03:35 PDT, Bob Clary [:bc:]
no flags Details
Testcase: Access via name still doesn't work? (1.53 KB, text/html)
2004-08-26 04:04 PDT, PikeUK
no flags Details
tests with final version of patch and patch to dom.properties (219.03 KB, application/zip)
2004-08-26 07:52 PDT, Bob Clary [:bc:]
no flags Details
Fix leaks, and hopefully Tp too. (8.39 KB, patch)
2004-08-27 11:42 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
Fix leaks, and hopefully Tp too. (previous patch had an extra if check in it that broke things) (8.41 KB, patch)
2004-08-27 12:35 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
Add missing JS_ClearScope() call, and fix nits from Brendan (8.24 KB, patch)
2004-08-27 13:16 PDT, Johnny Stenback (:jst, jst@mozilla.com)
brendan: review+
dbaron: superreview+
Details | Diff | Splinter Review
Patch that was checked in (with the NS_IF_ADDREF change). (8.63 KB, patch)
2004-08-27 14:17 PDT, Johnny Stenback (:jst, jst@mozilla.com)
asa: approval‑aviary+
Details | Diff | Splinter Review

Description Johnny Stenback (:jst, jst@mozilla.com) 2004-08-25 18:12:27 PDT
After lots of discussion and even more testing (thanks to Bob Clary endless
help), we've decided to add support for the IE feature that exposes elements by
their ID/NAME in the global scope. The need for this was realized after we added
the undetectable document.all support. Due to the somewhat large impact of this
change, we decided to do this for quirks mode documents only, and since we
decided that way, we also decided to pull the document.all support under the
same cover (i.e. document.all support will only be available in quirks mode
documents).

Patch coming up that does all that.
Comment 1 Hixie (not reading bugmail) 2004-08-25 18:19:39 PDT
Doesn't that go against normal Mozilla policy of not implementing quirks if they 
can be implemented in standards mode without affecting standards?
Comment 2 Johnny Stenback (:jst, jst@mozilla.com) 2004-08-25 18:30:52 PDT
Created attachment 157014 [details] [diff] [review]
Expose elements by their name/id in the global scope.
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2004-08-25 18:35:41 PDT
This feature does affect standard compliant code that for instance checks for
the existance of a global variable to set it only once. With this change, that
"varible" may now be a reference to an element in the document, and the code may
not work the way the developer intended.

That's the reason we decided to make this quirks only.
Comment 4 Johnny Stenback (:jst, jst@mozilla.com) 2004-08-25 19:11:30 PDT
Created attachment 157016 [details] [diff] [review]
diff -w
Comment 5 Brendan Eich [:brendan] 2004-08-25 19:25:11 PDT
Comment on attachment 157014 [details] [diff] [review]
Expose elements by their name/id in the global scope.

r+sr=me, I will post to the m.dom newsgroup -- people will be glad these are
quirks-only.

Need this in aviary, and may as well set approval1.7.3? to match how related
bugs's patches have been approved.

/be
Comment 6 Brendan Eich [:brendan] 2004-08-25 19:39:27 PDT
Wait, one more thing: do this only if !(flags & JSRESOLVE_QUALIFIED), so we take
no chance breaking good scripts that test 'if (!window.foo) window.foo = ...' in
a document with id or name=foo.

/be
Comment 7 Bob Clary [:bc:] 2004-08-25 20:16:46 PDT
Created attachment 157022 [details]
tests

compare trunk with patch against msie6, opera 7.54 and safari 3.2.1

no doctype vs. strict doctype comparison

msie does not change global references but adds a new #comment node 
note opera and safari do not change support
Comment 8 Bob Clary [:bc:] 2004-08-25 20:18:38 PDT
err, s/safari/konqueror/
Comment 9 Johnny Stenback (:jst, jst@mozilla.com) 2004-08-25 20:35:26 PDT
Created attachment 157023 [details] [diff] [review]
Only for non-qualified resolves, and warn when these references are used.
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2004-08-25 20:56:46 PDT
Fix checked in on the trunk and aviary branch.
Comment 11 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-08-26 01:57:16 PDT
This looks like it hurt Tp by about 1%.
Comment 12 Bob Clary [:bc:] 2004-08-26 03:35:16 PDT
Created attachment 157045 [details]
patch: add message to dom.properties
Comment 13 PikeUK 2004-08-26 04:04:19 PDT
Created attachment 157046 [details]
Testcase: Access via name still doesn't work?

Accessing an element through its id now works, but accessing through an
element's name still fails with "Error: <name> is not defined". (Apologies for
the spam if I have misunderstood the intent of this bug.)

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.2) Gecko/20040826 Firefox/0.9.1+

p.s. Also the JS console warning messages don't show up in Firefox, I filed Bug
256963 about that issue.
Comment 14 Bob Clary [:bc:] 2004-08-26 04:26:31 PDT
No, global name references do not work. See msie-mozilla-quirks.diff in the
tests attachment. 

The difference on the test between MSIE and the patched version of Mozilla is:

--- msie-quirks.txt	2004-08-25 23:06:20.187500000 -0400
+++ mozilla-quirks.txt	2004-08-25 23:06:06.421875000 -0400
@@ -1,22 +1,25 @@
 window[divid] DIV is global
 window[divinnerid] DIV is global
 window[aid] A is global
-window[aname] A is global
 window[formid] FORM is global
 window[formname] FORM is global
 window[formdivid] DIV is global
+window[inputtextid] INPUT is global
+window[inputradioid] INPUT is global
 window[outsideid] INPUT is global
-window[outsidename] INPUT is global
 document.all.item(divid) is DIV
+document.all.item(divname) is DIV
 document.all.item(divinnerid) is DIV
+document.all.item(divinnername) is DIV
 document.all.item(aid) is A
 document.all.item(aname) is A
 document.all.item(formid) is FORM
 document.all.item(formname) is FORM
 document.all.item(formdivid) is DIV
+document.all.item(formdivname) is DIV
 document.all.item(inputtextid) is INPUT
 document.all.item(inputtextname) is INPUT
-document.all.item(inputradioid) is undefined
+document.all.item(inputradioid) is INPUT
 document.all.item(inputradioname) is undefined
 document.all.item(outsideid) is INPUT
 document.all.item(outsidename) is INPUT
Comment 15 Bob Clary [:bc:] 2004-08-26 07:52:55 PDT
Created attachment 157063 [details]
tests with final version of patch and patch to dom.properties

				 1.7.3+   1.8a4

Total Pages all in Quirks	    73	     73
  
Total JavaScript Errors 	  1982	   1366
Unique JavaScript Errors	   125	     55

Total uses of document.all	     0	   1793
Unique uses of document.all	     0	    145

Total uses of Global References      0	    331
Unique uses of Global References     0	     31

Total 'not defined' errors	    57	      7
Unique 'not defined' errors	    29	      4

See the attached zip for full logs.
Comment 16 Robert Kaiser 2004-08-26 09:50:12 PDT
brad tinderbox shows a significant increase in leaks and refcnt leaks for the
time window where this and bug 256937 were checked in.
I don't know anything about those changes, but one of those two checkins looks
to have caused those leaks...
Comment 17 Johnny Stenback (:jst, jst@mozilla.com) 2004-08-26 10:29:29 PDT
I had a look at the leaks, and here's what's apparently now leaking that didn't
before my checkin, or some other checkin around that time (which seems really
unlikely):

--NEW-LEAKS-----------------------------------leaks------leaks%--------------
nsVoidArray                                    1544          -
nsDOMEventGroup                                   8          -
nsEventListenerManager                         5480          -
nsXBLKeyEventHandler                             96          -

No matter how hard I think about this, I can't figure out how my change could've
caused such new leaks. Unless I screwed up in my change, the only thing that
could cause leaks is the introduction of the new JS object in the global objects
prototype chain, and that means the leaks would come through JS roots, and of
the newly leaked objects, only nsXBLKeyEventHandler is something that is
directly scripted.

We used to leak no nsEventListenerManager objects, now we leak 137 of them.

dbaron, got any clues here?
Comment 18 Mike Shaver (:shaver -- probably not reading bugmail closely) 2004-08-27 06:31:24 PDT
And what of the Tp regression?
Comment 19 Robert O'Callahan (:roc) (email my personal email if necessary) 2004-08-27 07:57:46 PDT
I believe this should be backed out of Aviary.

It regressed Tp, it caused leaks, we have little time to stabilize it, and it
creates a 1.7 issue.
Comment 20 Johnny Stenback (:jst, jst@mozilla.com) 2004-08-27 10:08:24 PDT
Exactly what 1.7 issues does this cause?

I'm working on the Tp and leak issue, stay tuned.
Comment 21 Johnny Stenback (:jst, jst@mozilla.com) 2004-08-27 11:42:06 PDT
Created attachment 157175 [details] [diff] [review]
Fix leaks, and hopefully Tp too.
Comment 22 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-08-27 11:56:11 PDT
Comment on attachment 157175 [details] [diff] [review]
Fix leaks, and hopefully Tp too.

What calls InstallGlobalScopePolluter the first time around?
Comment 23 Johnny Stenback (:jst, jst@mozilla.com) 2004-08-27 12:01:50 PDT
GlobalWindowImpl::SetNewDocument() will call InstallGlobalScopePolluter() the
first time for each window.
Comment 24 Johnny Stenback (:jst, jst@mozilla.com) 2004-08-27 12:35:43 PDT
Created attachment 157178 [details] [diff] [review]
Fix leaks, and hopefully Tp too. (previous patch had an extra if check in it that broke things)
Comment 25 Johnny Stenback (:jst, jst@mozilla.com) 2004-08-27 12:59:29 PDT
Comment on attachment 157178 [details] [diff] [review]
Fix leaks, and hopefully Tp too. (previous patch had an extra if check in it that broke things)

Not quite there yet...
Comment 26 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-08-27 13:06:22 PDT
(In reply to comment #23)
> GlobalWindowImpl::SetNewDocument() will call InstallGlobalScopePolluter() the
> first time for each window.

But that looks like it happens only if there's an old one.
Comment 27 Johnny Stenback (:jst, jst@mozilla.com) 2004-08-27 13:16:06 PDT
Created attachment 157186 [details] [diff] [review]
Add missing JS_ClearScope() call, and fix nits from Brendan
Comment 28 Johnny Stenback (:jst, jst@mozilla.com) 2004-08-27 13:32:24 PDT
So the reason for this leak was (thanks to dbaron for finding out) that the new
code ended up holding on to the last document in a window at shutdown, and that
caused the document to be destroyed too late which caused its elements to not
clean up after themselves properly.

So this wasn't a "real" leak, just a shutdown leak.
Comment 29 Johnny Stenback (:jst, jst@mozilla.com) 2004-08-27 13:33:41 PDT
Duh, just realized that the last NS_ADDREF(doc) in InstallGlobalScopePolluter()
now needs to be an NS_IF_ADDREF(doc). Changed locally.
Comment 30 Brendan Eich [:brendan] 2004-08-27 13:41:08 PDT
Comment on attachment 157186 [details] [diff] [review]
Add missing JS_ClearScope() call, and fix nits from Brendan

With the needed NS_IF_RELEASE change, r=brendan@mozilla.org.

Man, I had forgetting about the mIsScopeClear madness.

/be
Comment 31 Mike Shaver (:shaver -- probably not reading bugmail closely) 2004-08-27 13:46:39 PDT
Please, for the sake of branch jockeys, attach an as-committed patch here when
you do the deed?

Muy thanko.
Comment 32 Johnny Stenback (:jst, jst@mozilla.com) 2004-08-27 14:17:40 PDT
Created attachment 157199 [details] [diff] [review]
Patch that was checked in (with the NS_IF_ADDREF change).
Comment 33 Daniel Luz 2004-08-27 16:42:01 PDT
What about exposing elements in the document object? I see way more cases of
"document.foo" than "foo" on the web.

I couldn't find a bug for that, should I file a new one or can this feature (for
IE compatibility) be part of this bug?
Comment 34 Johnny Stenback (:jst, jst@mozilla.com) 2004-08-27 16:44:14 PDT
That would be a separate bug, this is specifically for exposing elements by
their ID/NAME in the global scope.
Comment 35 Boris Zbarsky [:bz] (TPAC) 2004-08-29 21:57:28 PDT
> Exactly what 1.7 issues does this cause?

The fact that DOM support is different on 1.7 branch and aviary, while we're
trying to keep those Geckos in sync last I checked?

In other words, this patch probably needs to land on 1.7 too; if we're not
willing to do that we shouldn't be landing it on Aviary.
Comment 36 Asa Dotzler [:asa] 2004-08-30 17:09:15 PDT
Comment on attachment 157199 [details] [diff] [review]
Patch that was checked in (with the NS_IF_ADDREF change).

a=asa
Comment 37 Robert O'Callahan (:roc) (email my personal email if necessary) 2004-08-31 15:59:47 PDT
This has all got to go on the 1.7 branch, along with dependencies such as bug
257283.
Comment 38 Johnny Stenback (:jst, jst@mozilla.com) 2004-08-31 17:12:44 PDT
Bug 257602 filed to track the performance drop introduced by this change.
Comment 39 Johnny Stenback (:jst, jst@mozilla.com) 2004-09-15 20:35:40 PDT
Fixed on the 1.7 branch.
Comment 40 Andrew Schultz 2005-04-02 21:23:11 PST
*** Bug 271424 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.