Closed Bug 301498 Opened 16 years ago Closed 15 years ago

Enumeration on XPCNativeWrappers doesn't quite work

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.8beta5

People

(Reporter: bzbarsky, Assigned: brendan)

References

()

Details

(Keywords: fixed1.8)

Attachments

(2 files, 1 obsolete file)

See testcase URL.  I'm not sure whether calling the XPCScriptable hooks for
enumerate would help here or whether something else needs to happen...
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.
Blocks: 307294
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
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
Just attached a test case showing that this also happens on the regular content
[Window]
Taking.

/be
Assignee: general → brendan
Flags: blocking1.8b5+
Comment 4 has nothing to do with this bug, and in fact that testcase is
completely invalid.
Attached patch proposed fixSplinter Review
I haven't tested this.

/be
Attachment #196287 - Flags: superreview?(jst)
Attachment #196287 - Flags: review?(bzbarsky)
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.
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.
(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
Attachment #196203 - Attachment is obsolete: true
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
Status: NEW → ASSIGNED
OS: Linux → All
Priority: -- → P1
Hardware: PC → All
Whiteboard: have tested patch, waiting for review
Target Milestone: --- → mozilla1.8beta5
Comment on attachment 196287 [details] [diff] [review]
proposed fix

sr=jst
Attachment #196287 - Flags: superreview?(jst) → superreview+
Comment on attachment 196287 [details] [diff] [review]
proposed fix

sr=jst
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.
Attachment #196287 - Flags: review?(bzbarsky) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment on attachment 196287 [details] [diff] [review]
proposed fix

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

/be
Attachment #196287 - Flags: approval1.8b5?
Comment on attachment 196287 [details] [diff] [review]
proposed fix

Approved for 1.8b5 per bug meeting
Attachment #196287 - Flags: approval1.8b5? → approval1.8b5+
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
Flags: testcase?
Keywords: fixed1.8
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?
Same on trunk, Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1)
Gecko/20050924 Firefox/1.6a1.
(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
Flags: in-testsuite?
Whiteboard: have tested patch, waiting for review
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.