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

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
13 years ago
11 years ago

People

(Reporter: jst, Unassigned)

Tracking

({fixed-aviary1.0, fixed1.7.5})

Trunk
fixed-aviary1.0, fixed1.7.5
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 2 obsolete attachments)

22.26 KB, patch
brendan
: review+
brendan
: superreview+
Details | Diff | Splinter Review
19.70 KB, patch
Details | Diff | Splinter Review
7.01 KB, application/zip
Details
28.28 KB, patch
Details | Diff | Splinter Review
885 bytes, text/plain
Details
1.53 KB, text/html
Details
219.03 KB, application/zip
Details
8.24 KB, patch
brendan
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
8.63 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

13 years ago
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.
Doesn't that go against normal Mozilla policy of not implementing quirks if they 
can be implemented in standards mode without affecting standards?
(Reporter)

Comment 2

13 years ago
Created attachment 157014 [details] [diff] [review]
Expose elements by their name/id in the global scope.
(Reporter)

Comment 3

13 years ago
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.
(Reporter)

Updated

13 years ago
Attachment #157014 - Flags: superreview?(brendan)
Attachment #157014 - Flags: review?(brendan)
(Reporter)

Comment 4

13 years ago
Created attachment 157016 [details] [diff] [review]
diff -w
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
Attachment #157014 - Flags: superreview?(brendan)
Attachment #157014 - Flags: superreview+
Attachment #157014 - Flags: review?(brendan)
Attachment #157014 - Flags: review+
Attachment #157014 - Flags: approval1.7.3?
Attachment #157014 - Flags: approval-aviary+
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

13 years ago
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

13 years ago
err, s/safari/konqueror/
(Reporter)

Comment 9

13 years ago
Created attachment 157023 [details] [diff] [review]
Only for non-qualified resolves, and warn when these references are used.
(Reporter)

Comment 10

13 years ago
Fix checked in on the trunk and aviary branch.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Keywords: fixed-aviary1.0
Resolution: --- → FIXED
This looks like it hurt Tp by about 1%.

Comment 12

13 years ago
Created attachment 157045 [details]
patch: add message to dom.properties

Comment 13

13 years ago
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

13 years ago
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

13 years ago
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

13 years ago
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...
(Reporter)

Comment 17

13 years ago
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?

Updated

13 years ago
Blocks: 255514
And what of the Tp regression?
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.
(Reporter)

Comment 20

13 years ago
Exactly what 1.7 issues does this cause?

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

Comment 21

13 years ago
Created attachment 157175 [details] [diff] [review]
Fix leaks, and hopefully Tp too.
(Reporter)

Updated

13 years ago
Attachment #157175 - Flags: superreview?(brendan)
Attachment #157175 - Flags: review?(dbaron)
Comment on attachment 157175 [details] [diff] [review]
Fix leaks, and hopefully Tp too.

What calls InstallGlobalScopePolluter the first time around?
(Reporter)

Comment 23

13 years ago
GlobalWindowImpl::SetNewDocument() will call InstallGlobalScopePolluter() the
first time for each window.
(Reporter)

Comment 24

13 years ago
Created attachment 157178 [details] [diff] [review]
Fix leaks, and hopefully Tp too. (previous patch had an extra if check in it that broke things)
(Reporter)

Updated

13 years ago
Attachment #157175 - Attachment is obsolete: true
(Reporter)

Updated

13 years ago
Attachment #157175 - Flags: superreview?(brendan)
Attachment #157175 - Flags: review?(dbaron)
(Reporter)

Updated

13 years ago
Attachment #157178 - Flags: superreview?(brendan)
Attachment #157178 - Flags: review?(dbaron)
(Reporter)

Comment 25

13 years ago
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...
Attachment #157178 - Attachment is obsolete: true
Attachment #157178 - Flags: superreview?(brendan)
Attachment #157178 - Flags: review?(dbaron)
(Reporter)

Updated

13 years ago
Attachment #157178 - Flags: superreview?(brendan)
Attachment #157178 - Flags: review?(dbaron)
(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.
(Reporter)

Comment 27

13 years ago
Created attachment 157186 [details] [diff] [review]
Add missing JS_ClearScope() call, and fix nits from Brendan
(Reporter)

Comment 28

13 years ago
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.
(Reporter)

Comment 29

13 years ago
Duh, just realized that the last NS_ADDREF(doc) in InstallGlobalScopePolluter()
now needs to be an NS_IF_ADDREF(doc). Changed locally.
(Reporter)

Updated

13 years ago
Attachment #157178 - Flags: superreview?(brendan)
Attachment #157178 - Flags: review?(dbaron)
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
Attachment #157186 - Flags: review+
Attachment #157186 - Flags: superreview+
Please, for the sake of branch jockeys, attach an as-committed patch here when
you do the deed?

Muy thanko.
(Reporter)

Comment 32

13 years ago
Created attachment 157199 [details] [diff] [review]
Patch that was checked in (with the NS_IF_ADDREF change).

Comment 33

13 years ago
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?
(Reporter)

Comment 34

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

Comment 35

13 years ago
> 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.
(Reporter)

Updated

13 years ago
Attachment #157199 - Flags: approval-aviary?

Comment 36

13 years ago
Comment on attachment 157199 [details] [diff] [review]
Patch that was checked in (with the NS_IF_ADDREF change).

a=asa
Attachment #157199 - Flags: approval-aviary? → approval-aviary+
This has all got to go on the 1.7 branch, along with dependencies such as bug
257283.
(Reporter)

Updated

13 years ago
Blocks: 257602
(Reporter)

Comment 38

13 years ago
Bug 257602 filed to track the performance drop introduced by this change.
(Reporter)

Updated

13 years ago
Blocks: 257649
(Reporter)

Comment 39

13 years ago
Fixed on the 1.7 branch.
Keywords: fixed1.7.x

Updated

13 years ago
Attachment #157014 - Flags: approval1.7.x? → approval1.7.x+

Comment 40

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

Updated

11 years ago
Summary: Add support for exposing elements by their id/name in the global scope (for IE compat) → Expose elements by their id/name in the global scope (quirks mode only, for IE compat) (GlobalScopePolluter)
You need to log in before you can comment on or make changes to this bug.