Closed Bug 480185 Opened 11 years ago Closed 11 years ago

XPConnect shouldn't poke JS objects manually, should use JSAPI

Categories

(Core :: XPConnect, defect, P2)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

References

Details

(Keywords: fixed1.9.1)

Attachments

(2 files, 4 obsolete files)

Must remove OBJ_GET_PROPERTY and friends in favor of JSAPI calls so that we can trace reliably.
jorendorff also mentioned that anything that uses JSScopeProperty should be changed too.
XPConnect and caps (and maybe a few more places), heavily rely on STOBJ_GET_* to avoid the overhead of the JS API in cases where it's accessing properties in obj->fslots. Most of those accesses are to the private and class slots (class being in its own member now I guess), some are to parent and proto too. Need to figure out what to do in those cases I guess...
At risk of committing contributory incest, I'll note that you could probably split based on JS_ON_TRACE(cx)...
I don't think any of the fields we poke directly are jit-relevant. The jit only unpacks dslots afaict or we can make it so. xpconnect fiddles with fslots.
Attached patch Easy changes, v1 (obsolete) — Splinter Review
Here are the simple substitutions. Waiting on mrbkap for the more tricky changes needed in the wrappers code.
Attachment #366433 - Flags: review?(jst)
Attached patch Fix for XPCWrapper.cpp (obsolete) — Splinter Review
Attachment #366435 - Flags: superreview?(jst)
Attachment #366435 - Flags: review?(brendan)
Attachment #366435 - Flags: superreview?(jst) → superreview+
Attachment #366433 - Flags: superreview?(brendan)
Attachment #366433 - Flags: review?(jst)
Attachment #366433 - Flags: review+
Comment on attachment 366433 [details] [diff] [review]
Easy changes, v1

Brendan, are you cool with the #ifdef EXPORT_JS_API addition in jsobj.h?
Attachment #366433 - Flags: superreview?(brendan)
Comment on attachment 366433 [details] [diff] [review]
Easy changes, v1

This isn't ready for sr yet, need to integrate mrbkap's changes and apply them to xpcnativewrapper as well.
(In reply to comment #7)
> (From update of attachment 366433 [details] [diff] [review])
> Brendan, are you cool with the #ifdef EXPORT_JS_API addition in jsobj.h?

What's the rationale? I couldn't see anything defining this in either patch.

/be
Attached patch Fix for XPCWrapper.cpp v2 (obsolete) — Splinter Review
This patch depends on bug 482381.
Attachment #366435 - Attachment is obsolete: true
Attachment #366508 - Flags: superreview?(jst)
Attachment #366508 - Flags: review?(brendan)
Attachment #366435 - Flags: review?(brendan)
Attachment #366508 - Attachment is obsolete: true
Attachment #366508 - Flags: superreview?(jst)
Attachment #366508 - Flags: review?(brendan)
Attached patch Easy changes, v2 (obsolete) — Splinter Review
Attachment #366433 - Attachment is obsolete: true
Attachment #366939 - Flags: review?(jst)
Addresses some nits from mrbkap.
Attachment #366939 - Attachment is obsolete: true
Attachment #366945 - Flags: superreview?(jst)
Attachment #366945 - Flags: review?(mrbkap)
Attachment #366939 - Flags: review?(jst)
Attachment #366945 - Flags: superreview?(jst) → superreview+
Attachment #366945 - Flags: review?(mrbkap) → review+
Comment on attachment 366945 [details] [diff] [review]
Easy changes, v2.1

Please file a followup on investigating the forwarding code in nsWindowSH::AddProperty.
Landed both patches as revision 1f035bf7617b in mozilla-central.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
(In reply to comment #14)
> Please file a followup on investigating the forwarding code in
> nsWindowSH::AddProperty.

Filed bug 483495.
Attachment #366684 - Flags: approval1.9.1+
Comment on attachment 366684 [details] [diff] [review]
Fix for XPCWrapper.cpp v2+

Approving for 1.9.1 as this blocks mrbkap from merging his fix for bug 475864.
This needs to be a blocker for GC safety now that we're tracing across xpconnect stuff.
Flags: blocking1.9.1+
Priority: -- → P2
Blake landed this on 1.9.1, changeset 74a69006dbbc.
Keywords: fixed1.9.1
You need to log in before you can comment on or make changes to this bug.