The default bug view has changed. See this FAQ.

Enumeration on XPCNativeWrappers doesn't quite work

RESOLVED FIXED in mozilla1.8beta5

Status

()

Core
DOM
P1
normal
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: bz, Assigned: brendan)

Tracking

(Blocks: 1 bug, {fixed1.8})

Trunk
mozilla1.8beta5
fixed1.8
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

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
(Assignee)

Comment 2

12 years ago
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
(Assignee)

Comment 3

12 years ago
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

12 years ago
Created attachment 196203 [details]
Demonstrates problem in content context.

Comment 5

12 years ago
Just attached a test case showing that this also happens on the regular content
[Window]
(Assignee)

Comment 6

12 years ago
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.
(Assignee)

Comment 8

12 years ago
Created attachment 196287 [details] [diff] [review]
proposed fix

I haven't tested this.

/be
Attachment #196287 - Flags: superreview?(jst)
Attachment #196287 - Flags: review?(bzbarsky)

Comment 9

12 years ago
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.
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.
(Assignee)

Comment 11

12 years ago
(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
(Assignee)

Updated

12 years ago
Attachment #196203 - Attachment is obsolete: true
(Assignee)

Comment 12

12 years ago
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
(Assignee)

Updated

12 years ago
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+
(Assignee)

Comment 16

12 years ago
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
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 17

12 years ago
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 18

12 years ago
Comment on attachment 196287 [details] [diff] [review]
proposed fix

Approved for 1.8b5 per bug meeting
Attachment #196287 - Flags: approval1.8b5? → approval1.8b5+
(Assignee)

Comment 19

12 years ago
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

Comment 20

12 years ago
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

12 years ago
Same on trunk, Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1)
Gecko/20050924 Firefox/1.6a1.
(Assignee)

Comment 22

12 years ago
(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
(Assignee)

Updated

11 years ago
Flags: in-testsuite?
Whiteboard: have tested patch, waiting for review
You need to log in before you can comment on or make changes to this bug.