Closed
Bug 243591
Opened 21 years ago
Closed 13 years ago
Necko should build a proxy for notificationCallbacks that are used on background threads
Categories
(Core :: Networking, defect, P4)
Core
Networking
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: darin.moz, Unassigned)
References
Details
(Keywords: helpwanted)
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
Necko should build a proxy for notificationCallbacks that are used on background
threads. This will enable us to make nsDocShell NOT implement threadsafe addref
& release, which bryner recently found to be a top callsite for
PR_AtomicIncrement/Decrement.
Reporter | ||
Comment 1•21 years ago
|
||
This patch may need more testing.
could you possibly expose this to js? (the current code isn't xpcom safe, but
those changes would be minor).
Reporter | ||
Comment 3•21 years ago
|
||
what do you mean by "xpcom safe" exactly? if you found a bug in the code, can
you please point it out? as for exposing this to JS, yes, i suppose i could do
that. do you have a consumer in mind?
+ rv = NS_GetProxyForObject(NS_CURRENT_EVENTQ,
+ self->mIID,
+ obj,
+ PROXY_SYNC,
+ self->mResult);
the xpcom proxy hasn't been fixed to not do the wrong thing to poor defenseless
js objects.
as for consumers in mind? i think so. i certainly run js that does networking on
non main threads and can imagine wanting to play w/ notificationcallbacks, i'll
need to figure out what i'd do with them though.
Reporter | ||
Comment 5•21 years ago
|
||
> the xpcom proxy hasn't been fixed to not do the wrong thing to poor defenseless
> js objects.
timeless: it is possible that you could be anymore vague??? care to reference a
bug number or at least describe the problem you are talking about?
it's not really described in a bug, the symptoms can be found in bug 240462.
basically the proxy object calls QI on the wrong thread because it is afraid to
proxy over to do the QI (it's worried the object will disappear or something).
I've discussed this w/ dougt and the plan is to change the proxy object so that
it only proxies for a single interface. If the interface is nsIClassInfo then
it will let you access anything that classinfo exposes. Otherwise you'll have
to create additional proxy objects.
Reporter | ||
Updated•20 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8beta
Reporter | ||
Comment 7•20 years ago
|
||
timeless:
hmm, that's interesting. in the case of a sync proxy, i would think that we'd
be able to safely proxy the call to QueryInterface since we can just block the
calling thread until we get a PostCompleted event. perhaps a bug should be
filed on that. at any rate, i don't think it should hold up this patch.
Reporter | ||
Comment 8•20 years ago
|
||
changes include:
- fixes a bug in nsSocketTransport2.cpp
- make nsDocShell (actually nsDocLoader) implement non-threadsafe addref &
rel
- remove proxy object manager code from certain places in PSM
Attachment #148487 -
Attachment is obsolete: true
Reporter | ||
Comment 9•20 years ago
|
||
See also my comments in bug 31790. I think we might want to consider a
"recursive mode" for sync proxies created via nsIProxyObjectManager.
Comment 10•20 years ago
|
||
darin,
recursive mode is what I called "autoproxification". There use to be a patch
around that did this. I would rather be using this, then having a
nsInterfaceRequestorProxy.
Comment 11•20 years ago
|
||
dougt: hear, hear! Especially in light of nsIClassInfo, we should be automating
as much as we can. Where is that patch, anyway?
/be
Comment 12•20 years ago
|
||
IIRC, the code existed in the tree before I removed it. The first cut of the
core functionality was here:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsProxyEvent.cpp&branch=&root=/cvsroot&subdir=mozilla/xpcom/proxy/src&command=DIFF_FRAMESET&rev1=1.6&rev2=1.7
scoll down until you see a method named AutoProxyParameterList
Reporter | ||
Comment 13•20 years ago
|
||
dougt, brendan: i agree... thanks for the pointer. i'll look into it and see
what i can come up with.
Reporter | ||
Comment 14•20 years ago
|
||
I doubt I will have time to implement auto-proxification for Gecko 1.8. A shame
really. Help wanted.
Severity: normal → minor
Keywords: helpwanted
Priority: -- → P4
Target Milestone: mozilla1.8beta1 → mozilla1.8beta2
Reporter | ||
Updated•19 years ago
|
Target Milestone: mozilla1.8beta2 → mozilla1.9alpha
Reporter | ||
Updated•19 years ago
|
Assignee: darin → nobody
Status: ASSIGNED → NEW
QA Contact: benc → networking
Target Milestone: mozilla1.9alpha → ---
Going to go ahead and post this patch here as we may want to look at taking this some day soon (we need it for bug 372107). I totally agree that autoproxification is a great idea and that it should replace this fix, but since we've been without *any* fix for several years this may be worth it in the short term.
Comment 16•13 years ago
|
||
I am going to WONTFIX this because no background threads are accessing the callbacks any more, AFAICT. In particular, no PSM threads are accessing the callbacks off the main thread any more, after the SSL threading changes I made this quarter (including bug 674147, the PSM parts of bug 675221, and some bugs blocking those bugs).
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•