310742 - Properties of DOM objects are listed twice or 3 times

Status: VERIFIED FIXED

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

Description

[Gérard Talbot]

Steps to reproduce:
1- Load provided URL
2- 
a) Click the "Attribute and methods of the DOCUMENT.BODY object" button
or
b) Click the "Attribute and methods of the EVENT object" button

Actual results in Seamonkey 1.1a rv: 1.9a1 build 2005100105 and Firefox 1.4.1
rv: 1.8b5 build 20051001:
a) document.object attribute and methods are listed twice
b) event attribute and methods are listed 3 times

Expected results:
Attributes and methods should be listed once and only once.

Reproducible: Always

Note: 
- this must be a recent regression since I tried that demo not less than 2 weeks
ago.
- this could be a JS Engine component bug but I doubt it since the bug does not
happen for other objects
- if you try the DOCUMENT only button, you'll see that addEventListener is
listed twice

Reduced testcase coming up.

Comment 1

[Gérard Talbot]

Attachment: Testcase

Instructions: click the [Attributes and methods of the EVENT object ...] button


Expected results: only ABORT, ALT_MASK and AT_TARGET should be listed once and
only once.

Comment 2

[Ed Lee :Mardak]

Perhaps caused by bug 308856?

Last working version is Firefox 1.4 20050930
Confirmed regression on Firefox 1.4.1 20051001 (mac)

Comment 3

[Ed Lee :Mardak]

Backing out the 3 patches for bug 308856 seems to fix the problem... assuming I
applied the patches correctly. (But I was able to verify the issue without
patching then reverse patching and the testcase only shows 1 of each again.)

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

I'll look into this Monday at the latest.

Comment 5

[Asa Dotzler [:asa]]

blake, can you keep us up to date on your progress here? Time is very short for
this release.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

This was caused, specifically, by attachment 197494 [details] [diff] [review]. We are now not ignoring
properties that we were ignoring before (apparently, rightly). I'm still not
sure where these other properties are coming from, however.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Ugly, brutal, hack

I spoke to Brendan about this. The original patch to fix this problem was bogus
(and thus caused this bug). This patch fixes the issue without the regressions
but is an awful hack. Brendan said he'd think about this (and hopefully come up
with a better solution). I'm attaching this in case we don't find a better one
in the short-term (i.e., for the branch).

Comment 8

[Brendan Eich [:brendan]]

Taking from mrbkap so he can focus on WinXP drop shadow fixage.

/be

Comment 9

[Brendan Eich [:brendan]]

This is must-fix for Firefox 1.5 and other Gecko rv:1.8 apps.

/be

Comment 10

[Brendan Eich [:brendan]]

Attachment: pretty, gentle hack

No need for a goto and new label.  Note also that given the current JS API in
full, we can't fix this bug except by making the outerObject thing the special
case that makes this enumeration work.

In other words, if we don't elaborate the old logic here (!prop || obj != obj2,
particularly the latter), we don't enumerate split windows.  If we elaborate it
to allow any obj2 not on obj's prototype chain, we fix the window enumeration
case but get multiple id values enumerated per property for
XPCWrappedNativeProto cases.  The exact case we want is therefore peculiar to
split windows, which are the only implementors of JSExtendedClass.outerObject.

/be

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 198385 [details] [diff] [review]
pretty, gentle hack

Ah, much better! r=mrbkap

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 198385 [details] [diff] [review]
pretty, gentle hack

Self-approving, this is needed by the same token as 308856, and it's specific
to split window, so no risk to other objects, and easy to prove a fix for the
window case.

/be

Comment 13

[Brendan Eich [:brendan]]

Passes testcase, checked into trunk, hitting branch next.

/be

Comment 14

[Brendan Eich [:brendan]]

Fixed on branch and trunk.

/be

Comment 15

[Ed Lee :Mardak]

Verified. Testcase passed. (mac build)