301498 - Enumeration on XPCNativeWrappers doesn't quite work

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

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]]

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]]

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]

Attachment: Demonstrates problem in content context.

Comment 5

[Aaron Boodman]

Just attached a test case showing that this also happens on the regular content
[Window]

Comment 6

[Brendan Eich [:brendan]]

Taking.

/be

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment 4 has nothing to do with this bug, and in fact that testcase is
completely invalid.

Comment 8

[Brendan Eich [:brendan]]

Attachment: proposed fix

I haven't tested this.

/be

Comment 9

[Aaron Boodman]

Attachment: 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 [:bzbarsky]]

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]]

(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]]

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)]

Comment on attachment 196287 [details] [diff] [review]
proposed fix

sr=jst

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 196287 [details] [diff] [review]
proposed fix

sr=jst

Comment 15

[Boris Zbarsky [:bzbarsky]]

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]]

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]]

Comment on attachment 196287 [details] [diff] [review]
proposed fix

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

/be

Comment 18

[Mike Schroepfer]

Comment on attachment 196287 [details] [diff] [review]
proposed fix

Approved for 1.8b5 per bug meeting

Comment 19

[Brendan Eich [:brendan]]

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]

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]

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]]

(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