Last Comment Bug 301498 - Enumeration on XPCNativeWrappers doesn't quite work
: Enumeration on XPCNativeWrappers doesn't quite work
Status: RESOLVED FIXED
: fixed1.8
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.8beta5
Assigned To: Brendan Eich [:brendan]
: Hixie (not reading bugmail)
Mentors:
javascript: var x = 'fail'; for (var ...
Depends on: 296967
Blocks: 307294 281988
  Show dependency treegraph
 
Reported: 2005-07-20 14:47 PDT by Boris Zbarsky [:bz] (Out June 25-July 6)
Modified: 2006-08-17 18:41 PDT (History)
9 users (show)
brendan: blocking1.8b5+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Demonstrates problem in content context. (100 bytes, text/html)
2005-09-15 10:46 PDT, Aaron Boodman
no flags Details
proposed fix (3.50 KB, patch)
2005-09-15 22:34 PDT, Brendan Eich [:brendan]
bzbarsky: review+
jst: superreview+
mtschrep: approval1.8b5+
Details | Diff | Review
Updated testcase for comment 4 (167 bytes, text/html)
2005-09-16 11:46 PDT, Aaron Boodman
no flags Details

Description Boris Zbarsky [:bz] (Out June 25-July 6) 2005-07-20 14:47:39 PDT
See testcase URL.  I'm not sure whether calling the XPCScriptable hooks for
enumerate would help here or whether something else needs to happen...
Comment 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-07-29 14:12:21 PDT
So I thought about it, and we'd somehow need to expose all of our XPConnectified
stuff when enumerating.  So we'd need to do our own enumerate hook and duplicate
most of what the JSeng does in its hook in addition to whatever we need to do
for the XPConnectified stuff.  At least if I read the JSEng code correctly.
Comment 2 Brendan Eich [:brendan] 2005-09-07 10:59:08 PDT
All we need to do is poke the wrapped object to enumerate its enumerable
properties.  We should be careful about JSCLASS_NEW_ENUMERATE (we're not testing
that at all, currently).  Then once we've made the wrapped object stop being
lazy we can reflect its enumerable properties into the XPCNativeWrapper.

/be
Comment 3 Brendan Eich [:brendan] 2005-09-07 14:34:29 PDT
It seems to me JS_Enumerate on the wrapped object should do the trick.  We don't
need to copy any internal magic out of the JS engine into XPCNativeWrapper.cpp.

/be
Comment 4 Aaron Boodman 2005-09-15 10:46:46 PDT
Created attachment 196203 [details]
Demonstrates problem in content context.
Comment 5 Aaron Boodman 2005-09-15 10:47:23 PDT
Just attached a test case showing that this also happens on the regular content
[Window]
Comment 6 Brendan Eich [:brendan] 2005-09-15 11:21:19 PDT
Taking.

/be
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-09-15 21:52:26 PDT
Comment 4 has nothing to do with this bug, and in fact that testcase is
completely invalid.
Comment 8 Brendan Eich [:brendan] 2005-09-15 22:34:39 PDT
Created attachment 196287 [details] [diff] [review]
proposed fix

I haven't tested this.

/be
Comment 9 Aaron Boodman 2005-09-16 11:46:46 PDT
Created attachment 196348 [details]
Updated testcase for comment 4

Updating testcase in comment 4, which was invalid.

Nevertheless the bug I was trying to demonstrate *does* exist and this new
testcase shows it.

This worked in previous firefoxes. I just tested on 1.0, for instance. 

I'm not sure if it's the same as this bug, but Brendan suggested that it was.
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-09-16 12:03:54 PDT
No, that's a different bug, one having to do with splitwindow, not with
XPCNativeWrapper. There's no way brendan's patch in this bug could fix that problem.

Please file a new bug on that issue, cc brendan, jst, and myself.
Comment 11 Brendan Eich [:brendan] 2005-09-16 12:26:20 PDT
(In reply to comment #9)
> I'm not sure if it's the same as this bug, but Brendan suggested that it was.

Sorry for the confusion -- I'm not sure which bug I said was which, but it is
great to get them on file, and quickly -- time for 1.5b2 is running out and we
need to fix all of these regressions.  Thanks,

/be
Comment 12 Brendan Eich [:brendan] 2005-09-16 16:53:07 PDT
From IRC:

<brendan> bz_away, mrbkap: my XPCNativeWrapper enumeration fix works
<brendan> AHEM!
<brendan> !!!!!@@@@@#####$$$$$ID QueryInterface interfaces interfacesByID
classes classesByID stack results manager utils Exception Constructor
isSuccessCode lookupMethod reportError
<brendan> from this addition to nsSample.js:
<brendan> dump("AHEM!\n");
<brendan> dump("!!!!!@@@@@#####$$$$$" + (function () {
<brendan>     try {
<brendan>         var w = Components;
<brendan>         var x = new XPCNativeWrapper(w);
<brendan>         var s = "";
<brendan>         for (var i in x)
<brendan>             s += (s ? " " : "") + i;
<brendan>         return s;
<brendan>     } catch (e) {
<brendan>         return "COUGH! " + e;
<brendan>     }
<brendan> })() + "\n");

/be
Comment 13 Johnny Stenback (:jst, jst@mozilla.com) 2005-09-17 09:09:06 PDT
Comment on attachment 196287 [details] [diff] [review]
proposed fix

sr=jst
Comment 14 Johnny Stenback (:jst, jst@mozilla.com) 2005-09-18 20:06:06 PDT
Comment on attachment 196287 [details] [diff] [review]
proposed fix

sr=jst
Comment 15 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-09-18 20:30:52 PDT
Comment on attachment 196287 [details] [diff] [review]
proposed fix

>Index: XPCNativeWrapper.cpp
>+//      convert, (cx, obj, type, vp)
> //
>-// in the call from XPC_NW_Enumerate, for example.
>+// in the call from XPC_NW_BYPASS, for example.

s/BYPASS/Convert/ or something here?  This comment doesn't make sense to me as
written.

r=bzbarsky with that.
Comment 16 Brendan Eich [:brendan] 2005-09-18 23:32:10 PDT
Thanks, fixed that and tweaked another comment.  This should go onto the branch
pretty soon, it's a straightforward fix that doesn't need a lot of baking.

/be
Comment 17 Brendan Eich [:brendan] 2005-09-18 23:52:21 PDT
Comment on attachment 196287 [details] [diff] [review]
proposed fix

This is needed, given automatic wrapping, for backward compatibility.

/be
Comment 18 Mike Schroepfer 2005-09-19 14:43:04 PDT
Comment on attachment 196287 [details] [diff] [review]
proposed fix

Approved for 1.8b5 per bug meeting
Comment 19 Brendan Eich [:brendan] 2005-09-19 15:58:54 PDT
A testcase based on nsSample.js can be generated from comment 12, perhaps with
some cleanup to my Popeye-style cussing (needed to cut through all the noise in
my debug build's stdout/stderr ;-).

Fixed on the 1.8 branch.

/be
Comment 20 Jesse Ruderman 2005-09-24 20:31:27 PDT
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b5) Gecko/20050924
Firefox/1.4

I see "fail" when I click the URL field link, and the testcase in comment 9
doesn't say "monkey".  Should I reopen this bug?
Comment 21 Jesse Ruderman 2005-09-24 20:32:40 PDT
Same on trunk, Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1)
Gecko/20050924 Firefox/1.6a1.
Comment 22 Brendan Eich [:brendan] 2005-09-24 21:11:32 PDT
(In reply to comment #20)
> Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b5) Gecko/20050924
> Firefox/1.4
> 
> I see "fail" when I click the URL field link, and the testcase in comment 9
> doesn't say "monkey".  Should I reopen this bug?

No.  That testcase, again, is about split window enumeration (bug 308856), which
is not XPCNativeWrapper enumeration (this bug).

/be

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