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)

defect

Tracking

()

RESOLVED WONTFIX

People

(Reporter: darin.moz, Unassigned)

References

Details

(Keywords: helpwanted)

Attachments

(2 files, 1 obsolete file)

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.
Attached patch v1 patch (obsolete) (deleted) — Splinter Review
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).
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.
> 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.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8beta
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.
Attached patch v2 patch (deleted) — Splinter Review
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
See also my comments in bug 31790. I think we might want to consider a "recursive mode" for sync proxies created via nsIProxyObjectManager.
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.
dougt: hear, hear! Especially in light of nsIClassInfo, we should be automating as much as we can. Where is that patch, anyway? /be
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
dougt, brendan: i agree... thanks for the pointer. i'll look into it and see what i can come up with.
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
Target Milestone: mozilla1.8beta2 → mozilla1.9alpha
Blocks: 289613
No longer blocks: 289613
Assignee: darin → nobody
Status: ASSIGNED → NEW
QA Contact: benc → networking
Target Milestone: mozilla1.9alpha → ---
Attached patch Updated 1.9 trunk patch (deleted) — Splinter Review
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.
Blocks: 454966
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.

Attachment

General

Creator:
Created:
Updated:
Size: