Last Comment Bug 310742 - Properties of DOM objects are listed twice or 3 times
: Properties of DOM objects are listed twice or 3 times
Status: VERIFIED FIXED
: fixed1.8, regression
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: 1.8 Branch
: All All
: P1 normal (vote)
: mozilla1.8beta5
Assigned To: Brendan Eich [:brendan]
:
Mentors:
http://www.gtalbot.org/DHTMLSection/L...
Depends on:
Blocks: 308856
  Show dependency treegraph
 
Reported: 2005-10-01 19:20 PDT by Gérard Talbot
Modified: 2008-07-31 02:43 PDT (History)
5 users (show)
brendan: blocking1.8b5+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (3.67 KB, text/html)
2005-10-01 19:33 PDT, Gérard Talbot
no flags Details
Ugly, brutal, hack (2.90 KB, patch)
2005-10-03 16:17 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
pretty, gentle hack (2.77 KB, patch)
2005-10-03 17:25 PDT, Brendan Eich [:brendan]
mrbkap: review+
brendan: approval1.8b5+
Details | Diff | Splinter Review

Description Gérard Talbot 2005-10-01 19:20:42 PDT
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 2005-10-01 19:33:24 PDT
Created attachment 198181 [details]
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 2005-10-01 21:40:48 PDT
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 2005-10-02 00:58:37 PDT
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) 2005-10-02 02:27:38 PDT
I'll look into this Monday at the latest.
Comment 5 Asa Dotzler [:asa] 2005-10-03 11:05:16 PDT
blake, can you keep us up to date on your progress here? Time is very short for
this release.
Comment 6 Blake Kaplan (:mrbkap) 2005-10-03 11:08:38 PDT
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) 2005-10-03 16:17:02 PDT
Created attachment 198376 [details] [diff] [review]
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] 2005-10-03 17:20:20 PDT
Taking from mrbkap so he can focus on WinXP drop shadow fixage.

/be
Comment 9 Brendan Eich [:brendan] 2005-10-03 17:21:27 PDT
This is must-fix for Firefox 1.5 and other Gecko rv:1.8 apps.

/be
Comment 10 Brendan Eich [:brendan] 2005-10-03 17:25:28 PDT
Created attachment 198385 [details] [diff] [review]
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) 2005-10-03 17:29:54 PDT
Comment on attachment 198385 [details] [diff] [review]
pretty, gentle hack

Ah, much better! r=mrbkap
Comment 12 Brendan Eich [:brendan] 2005-10-03 17:34:24 PDT
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] 2005-10-03 17:39:55 PDT
Passes testcase, checked into trunk, hitting branch next.

/be
Comment 14 Brendan Eich [:brendan] 2005-10-03 17:55:09 PDT
Fixed on branch and trunk.

/be
Comment 15 Ed Lee :Mardak 2005-10-03 20:04:09 PDT
Verified. Testcase passed. (mac build)

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