Closed
Bug 1515884
Opened 6 years ago
Closed 6 years ago
Remove unused XPCWrappedJS nsIPropertyBag implementation
Categories
(Core :: XPConnect, enhancement)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla66
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: jandem, Assigned: kmag)
Details
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
See bug 1514210 comment 12 and 13.
Assignee | ||
Comment 1•6 years ago
|
||
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)
Assignee | ||
Updated•6 years ago
|
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
Assignee | ||
Comment 3•6 years ago
|
||
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.
Reporter | ||
Comment 4•6 years ago
|
||
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.
Assignee | ||
Comment 5•6 years ago
|
||
(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...
Comment 6•6 years ago
|
||
(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.
Reporter | ||
Comment 7•6 years ago
|
||
(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 :/
Assignee | ||
Comment 8•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6e4acbfd9cc1f7fd7beffc9172fa6e6297ac180 Bug 1515884: Remove unused XPCWrappedJS nsIPropertyBag implementation. r=bholley
Comment 9•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e6e4acbfd9cc
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox66:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in
before you can comment on or make changes to this bug.
Description
•