Finish getting rid of nsXPCWrappedJSClass

RESOLVED FIXED in Firefox 68

Status

()

task
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

3 months ago

After bug 1540301 lands, nsXPCWrappedJSClass will just be a pile of static methods. These should really be methods for nsXPCWrappedJS. Some of them aren't even really static methods. It would be nice if this could somehow be done while preserving the blame history.

Assignee

Updated

3 months ago
Assignee: nobody → continuation
Assignee

Comment 1

3 months ago

I think I'll just leave these methods in XPCWrappedJSClass.cpp to preserve the blame. That's a bit goofy, but in the modern era of SearchFox it doesn't seem too terrible. I also added comments at the point of declaration about where the definitions live.

Assignee

Comment 2

3 months ago

I'm going to wait until the rest of the patch stack settles out before uploading this for review.

Assignee

Updated

3 months ago
Type: defect → task
Assignee

Comment 3

3 months ago

I changed DelegatedQueryInterface and CallMethod to be non-static
methods rather than taking an explicit |self| parameter.

I did a tiny bit of cleanup in the nsIXPConnectJSObjectHolder case of
DelegatedQueryInterface().

There is already a method nsXPCWrappedJS::CallMethod() with the same
signature, but it is a shim, so I inlined it into the version in
XPCWrappedJSClass.cpp.

I also fixed up a few comments that mention nsXPCWrappedJSClass.

I renamed DebugDump() to DebugDumpInterfaceInfo() to be more
informative.

Comment 4

3 months ago
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5937ad352b2d
Eliminate nsXPCWrappedJSClass by moving its methods into nsXPCWrappedJS. r=bzbarsky
Assignee

Comment 6

3 months ago

Looks like a stack overflow, calling from nsCOMPtr<nsIXPConnectJSObjectHolder>::Assert_NoQueryNeeded() to nsXPCWrappedJS::QueryInterface(nsID const&, void**) to nsXPCWrappedJS::DelegatedQueryInterface(nsID const&, void**) and back again.

Flags: needinfo?(continuation)
Assignee

Comment 7

3 months ago

I think the problem is that I changed NS_ADDREF(self). to nsCOMPtr<nsIXPConnectJSObjectHolder> rval(this). The latter hits nsCOMPtr<> asserts that end up back inside the QI. I'll revert that part and add a comment...

Comment 8

3 months ago
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2d61a31bec7d
Eliminate nsXPCWrappedJSClass by moving its methods into nsXPCWrappedJS. r=bzbarsky

Comment 9

3 months ago
bugherder
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.