256932 - Expose elements by their id/name in the global scope (quirks mode only, for IE compat) (GlobalScopePolluter)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Johnny Stenback (:jst)]

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)]

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)]

Attachment: Expose elements by their name/id in the global scope.

Comment 3

[Johnny Stenback (:jst)]

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)]

Attachment: diff -w

Comment 5

[Brendan Eich [:brendan]]

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]]

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] (inactive)]

Attachment: 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] (inactive)]

err, s/safari/konqueror/

Comment 9

[Johnny Stenback (:jst)]

Attachment: Only for non-qualified resolves, and warn when these references are used.

Comment 10

[Johnny Stenback (:jst)]

Fix checked in on the trunk and aviary branch.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This looks like it hurt Tp by about 1%.

Comment 12

[Bob Clary [:bc] (inactive)]

Attachment: patch: add message to dom.properties

Comment 13

[PikeUK]

Attachment: 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] (inactive)]

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] (inactive)]

Attachment: 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]

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)]

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)]

And what of the Tp regression?

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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)]

Exactly what 1.7 issues does this cause?

I'm working on the Tp and leak issue, stay tuned.

Comment 21

[Johnny Stenback (:jst)]

Attachment: Fix leaks, and hopefully Tp too.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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)]

GlobalWindowImpl::SetNewDocument() will call InstallGlobalScopePolluter() the
first time for each window.

Comment 24

[Johnny Stenback (:jst)]

Attachment: Fix leaks, and hopefully Tp too. (previous patch had an extra if check in it that broke things)

Comment 25

[Johnny Stenback (:jst)]

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-4, no longer working on Mozilla)]

(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)]

Attachment: Add missing JS_ClearScope() call, and fix nits from Brendan

Comment 28

[Johnny Stenback (:jst)]

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)]

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]]

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=[e-mail].

Man, I had forgetting about the mIsScopeClear madness.

/be

Comment 31

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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)]

Attachment: Patch that was checked in (with the NS_IF_ADDREF change).

Comment 33

[Daniel Luz]

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)]

That would be a separate bug, this is specifically for exposing elements by
their ID/NAME in the global scope.

Comment 35

[Boris Zbarsky [:bzbarsky]]

> 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]]

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)]

This has all got to go on the 1.7 branch, along with dependencies such as bug
257283.

Comment 38

[Johnny Stenback (:jst)]

Bug 257602 filed to track the performance drop introduced by this change.

Comment 39

[Johnny Stenback (:jst)]

Fixed on the 1.7 branch.

Comment 40

[Andrew Schultz]

*** Bug 271424 has been marked as a duplicate of this bug. ***