Closed Bug 1515884 Opened 5 years ago Closed 5 years ago

Remove unused XPCWrappedJS nsIPropertyBag implementation

Categories

(Core :: XPConnect, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: jandem, Assigned: kmag)

Details

Attachments

(1 file)

See bug 1514210 comment 12 and 13.
Bobby, it doesn't look like this feature is used at all anymore, aside from [1], which just uses it as a hack to figure out if something is an XPCWrappedJS. Would you be OK with just removing support?


[1]: https://searchfox.org/mozilla-central/rev/9528360768d81b1fc84258b5fb3601b5d4f40076/netwerk/protocol/http/nsHttpConnectionMgr.cpp#543-547
Flags: needinfo?(bobbyholley)
Happy to get rid of it. Thanks!
Flags: needinfo?(bobbyholley)
Assignee: nobody → kmaglione+bmo
Summary: Only return XPCWrappedJS nsIPropertyBag implementation if the wrapped object doesn't query to nsIPropertyBag on its own → Remove unused XPCWrappedJS nsIPropertyBag implementation
This helper code is currently unused, and presents a pretty significant
footgun for any JS object which implements nsIPropertyBag itself.

When those objects are first queried to nsIWritablePropertyBag, they behave as
expected, returning the JS-implemented nsIPropertyBag methods. But when
they're first queried to nsIPropertyBag, they use the XPCWrappedNative stubs,
which don't behave as expected.
Note that nsIUpdate uses nsIPropertyBag (on WrappedJS) for the backgroundInterval field and nsIWritablePropertyBag for extra attributes. I'm fixing that in bug 1515611 by removing that field but you might run into issues now.
(In reply to Jan de Mooij [:jandem] from comment #4)
> Note that nsIUpdate uses nsIPropertyBag (on WrappedJS) for the
> backgroundInterval field and nsIWritablePropertyBag for extra attributes.
> I'm fixing that in bug 1515611 by removing that field but you might run into
> issues now.

Yeah, I just ran into that. A couple of pieces of code rely on the XPCWrappedJS implementation of nsIPropertyBag for that property, and don't work with the JS-implemented one. Everything else relies on the JS-implementation version, and doesn't work with the XPCWrappedJS version...
(In reply to Jan de Mooij [:jandem] from comment #4)
> Note that nsIUpdate uses nsIPropertyBag (on WrappedJS) for the
> backgroundInterval field and nsIWritablePropertyBag for extra attributes.
> I'm fixing that in bug 1515611 by removing that field but you might run into
> issues now.

It goes without saying, but please be extremely careful touching the updater, and get review from the relevant people. Breaking the updater means we lose all our users.
(In reply to Bobby Holley (:bholley) from comment #6)
> It goes without saying, but please be extremely careful touching the
> updater, and get review from the relevant people. Breaking the updater means
> we lose all our users.

Yeah I had a patch to fix it but rstrong suggested removing the field because we apparently don't need it. It does make me a bit uneasy making changes there for the reason you mentioned :/
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6e4acbfd9cc1f7fd7beffc9172fa6e6297ac180
Bug 1515884: Remove unused XPCWrappedJS nsIPropertyBag implementation. r=bholley
https://hg.mozilla.org/mozilla-central/rev/e6e4acbfd9cc
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: