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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
15 years ago
4 months ago

People

(Reporter: jst, Unassigned)

Tracking

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

Trunk
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
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?
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.
Attachment #157014 - Flags: superreview?(brendan)
Attachment #157014 - Flags: review?(brendan)
Posted patch diff -wSplinter Review
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
Posted file 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
err, s/safari/konqueror/
Fix checked in on the trunk and aviary branch.
Status: NEW → RESOLVED
Closed: 15 years ago
Keywords: fixed-aviary1.0
Resolution: --- → FIXED
This looks like it hurt Tp by about 1%.
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.
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
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.
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...
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?
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.
Exactly what 1.7 issues does this cause?

I'm working on the Tp and leak issue, stay tuned.
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?
GlobalWindowImpl::SetNewDocument() will call InstallGlobalScopePolluter() the
first time for each window.
Attachment #157175 - Attachment is obsolete: true
Attachment #157175 - Flags: superreview?(brendan)
Attachment #157175 - Flags: review?(dbaron)
Attachment #157178 - Flags: superreview?(brendan)
Attachment #157178 - Flags: review?(dbaron)
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)
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.
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.
Duh, just realized that the last NS_ADDREF(doc) in InstallGlobalScopePolluter()
now needs to be an NS_IF_ADDREF(doc). Changed locally.
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+
Please, for the sake of branch jockeys, attach an as-committed patch here when
you do the deed?

Muy thanko.
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?
That would be a separate bug, this is specifically for exposing elements by
their ID/NAME in the global scope.
> 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.
Attachment #157199 - Flags: approval-aviary?
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.
Blocks: 257602
Bug 257602 filed to track the performance drop introduced by this change.
Blocks: 257649
Fixed on the 1.7 branch.
Keywords: fixed1.7.x
Attachment #157014 - Flags: approval1.7.x? → approval1.7.x+
*** Bug 271424 has been marked as a duplicate of this bug. ***
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)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.