Closed Bug 306482 Opened 19 years ago Closed 19 years ago

Chatzilla accesses DOM objects from off the UI thread

Categories

(Other Applications :: ChatZilla, defect)

Product:
Other Applications
Component:
ChatZilla
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: rginda)

Details

(Whiteboard: [cz-0.9.69])

Attachments

(1 file)

Use main thread to work around DOM suckage
970 bytes, patch
rginda
: review+
Details | Diff | Splinter Review 
STEPS TO REPRODUCE: start chatzilla

ACTUAL RESULTS:
###!!! ASSERTION: nsGlobalWindow not thread-safe: '_mOwningThread.GetThread() ==
PR_GetCurrentThread()', file
../../../../mozilla/dom/src/base/nsGlobalWindow.cpp, line 481

EXPECTED RESULTS: no asserts

OTHER INFORMATION:  The problem is that chatzilla does: 

  132  var dnsRecord = this._dnsSvc.asyncResolve(host, false, listener, null);

in dcc.js.  The documentation for asyncResolve's last arg says:


61   * @param aListenerEventTarget
62   *        optional parameter (may be null).  if non-null, this parameter
63   *        specifies the nsIEventTarget of the thread on which the listener's
64   *        onLookupComplete should be called.  however, if this parameter is
65   *        null, then onLookupComplete will be called on an unspecified
66   *        thread (possibly recursively).

In other words, Chatzilla should be passing in the UI thread event queue here,
not null.
Please be careful what you accuse. null was passed specifically because the 
code being called does not touch any DOM stuff knowingly. You'll need to 
identify EXACTLY what part of the lookup callback touches DOM before I care.
Any function from a script loaded in a DOM context (loaded from a window, not a
JS component) must be main-thread only, because the global object is the DOM
window object. And please, bz is not "accusing" anything, he's filing a fairly
simple threadsafety bug report based on an assertion.
FWIW, here's a quick code snipet that can be used to get the proper value for
the nsIEventTarget parameter:

  var eqs =
    Components.classes["@mozilla.org/event-queue-service;1"].
    getService(Components.interfaces.nsIEventQueueService);
  var eq = eqs.getSpecialEventQueue(eqs.CURRENT_THREAD_EVENT_QUEUE);
I would have thought simply doing the steps to reproduce would have shown you
the problem, but since you insist here is the stack:

#0  0xb7f19f07 in nsDebug::Assertion (aStr=0xb6af19c4 "nsGlobalWindow not
thread-safe", 
    aExpr=0xb6af1990 "_mOwningThread.GetThread() == PR_GetCurrentThread()", 
    aFile=0xb6af184c "../../../../mozilla/dom/src/base/nsGlobalWindow.cpp",
aLine=481)
    at nsDebug.cpp:108
#1  0xb688dbd1 in nsGlobalWindow::AddRef (this=0xb22abb00)
    at ../../../../mozilla/dom/src/base/nsGlobalWindow.cpp:481
#2  0xb68a5f91 in nsGlobalChromeWindow::AddRef (this=0xb22abb00)
    at ../../../../mozilla/dom/src/base/nsGlobalWindow.cpp:6845
#3  0xb688db2b in nsGlobalWindow::QueryInterface (this=0xb22abb00,
aIID=@0xb784ab10, 
    aInstancePtr=0xb0cfce80) at
../../../../mozilla/dom/src/base/nsGlobalWindow.cpp:478
#4  0xb68a5f46 in nsGlobalChromeWindow::QueryInterface (this=0xb22abb00, 
    aIID=@0xb784ab10, aInstancePtr=0xb0cfcec0)
    at ../../../../mozilla/dom/src/base/nsGlobalWindow.cpp:6843
#5  0xb7f192dc in nsQueryInterface::operator() (this=0xb0cfced4, aIID=@0xb784ab10, 
    answer=0xb0cfcec0) at nsCOMPtr.cpp:47
#6  0xb784381f in nsCOMPtr<nsIScriptObjectPrincipal>::assign_from_qi
(this=0xb0cfcf20, 
    qi={mRawPtr = 0xb22abb28}, aIID=@0xb784ab10)
    at ../../dist/include/xpcom/nsCOMPtr.h:1232
#7  0xb7841f06 in nsCOMPtr<nsIScriptObjectPrincipal>::operator=
(this=0xb0cfcf20, rhs=
      {mRawPtr = 0xb22abb28}) at ../../dist/include/xpcom/nsCOMPtr.h:731
#8  0xb783a8a2 in nsScriptSecurityManager::doGetObjectPrincipal (aCx=0x8524830, 
    aObj=0xb2256618) at ../../../mozilla/caps/src/nsScriptSecurityManager.cpp:2100
#9  0xb783a3c8 in nsScriptSecurityManager::GetFunctionObjectPrincipal
(cx=0x8524830, 
    obj=0xb1a31418, fp=0xb0cfdf20, rv=0xb0cfd2bc)
    at ../../../mozilla/caps/src/nsScriptSecurityManager.cpp:1961
#10 0xb783a497 in nsScriptSecurityManager::GetFramePrincipal (cx=0x8524830, 
    fp=0xb0cfdf20, rv=0xb0cfd2bc)
    at ../../../mozilla/caps/src/nsScriptSecurityManager.cpp:1985
#11 0xb783a58e in nsScriptSecurityManager::GetPrincipalAndFrame (cx=0x8524830, 
    frameResult=0xb0cfd020, rv=0xb0cfd2bc)
    at ../../../mozilla/caps/src/nsScriptSecurityManager.cpp:2016
#12 0xb783a73b in nsScriptSecurityManager::GetSubjectPrincipal (cx=0x8524830, 
    rv=0xb0cfd2bc) at ../../../mozilla/caps/src/nsScriptSecurityManager.cpp:2058
#13 0xb783555f in nsScriptSecurityManager::CheckPropertyAccessImpl
(this=0x8233d38,    aAction=0, aCallContext=0xb0cfd5c0, cx=0x8524830,
aJSObject=0xb1a31660, 
    aObj=0x85413d8, aTargetURI=0x0, aClassInfo=0x0, aClassName=0x0, 
    aProperty=-1305190004, aCachedClassPolicy=0x0)
    at ../../../mozilla/caps/src/nsScriptSecurityManager.cpp:606
#14 0xb783d486 in nsScriptSecurityManager::CanAccess (this=0x8233d38, aAction=0, 
    aCallContext=0xb0cfd5c0, cx=0x8524830, aJSObject=0xb1a31660, aObj=0x85413d8, 
    aClassInfo=0x0, aPropertyName=-1305190004, aPolicy=0x0)
    at ../../../mozilla/caps/src/nsScriptSecurityManager.cpp:2800
#15 0xb79a584c in XPCWrappedNative::CallMethod (ccx=@0xb0cfd5c0, mode=CALL_METHOD)
    at ../../../../../mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:1792
#16 0xb79b0e33 in XPC_WN_CallMethod (cx=0x8524830, obj=0xb1a31660, argc=0, 
    argv=0x85a8fbc, vp=0xb0cfd770)
    at ../../../../../mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1402
#17 0xb7e13a5f in js_Invoke (cx=0x8524830, argc=0, flags=0)
    at ../../../mozilla/js/src/jsinterp.c:1174
#18 0xb7e23442 in js_Interpret (cx=0x8524830, pc=0xb1e813c5 ":", result=0xb0cfdf0c)
    at ../../../mozilla/js/src/jsinterp.c:3469
#19 0xb7e13ae1 in js_Invoke (cx=0x8524830, argc=3, flags=2)
    at ../../../mozilla/js/src/jsinterp.c:1194
#20 0xb799ebf8 in nsXPCWrappedJSClass::CallMethod (this=0xb1add638,
wrapper=0xb1743858, 
    methodIndex=3, info=0xb1adcbc8, nativeParams=0xb0cfe2a0)
    at ../../../../../mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp:1339
#21 0xb799790d in nsXPCWrappedJS::CallMethod (this=0xb1743858, methodIndex=3, 
    info=0xb1adcbc8, params=0xb0cfe2a0)
    at ../../../../../mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp:461
#22 0xb7fc5096 in PrepareAndDispatch (methodIndex=3, self=0xb1743858,
args=0xb0cfe344)
    at
../../../../../../../mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_gcc_x86_unix.cpp:100
#23 0xb770a9bd in nsDNSAsyncRequest::OnLookupComplete (this=0xb1743890, 
    resolver=0x8253950, hostRecord=0xb17438c8, status=0)
    at ../../../../mozilla/netwerk/dns/src/nsDNSService2.cpp:223

The JS stack looks like:

0 [native frame]
1 _onLookupComplete(request = [xpconnect wrapped nsICancelable @ 0x85e0560
(native @ 0xb3c6a6c4)], record = [xpconnect wrapped nsIDNSRecord @ 0x85dd6b0
(native @ 0x85eb8d0)], status = 0) ["chrome://chatzilla/content/lib/js/dcc.js":125]
    this = [object Object]
2 [native frame]

So as Benjamin pointed out the problem is that any call through XPConnect (the
hasMore call here, for example) will try to look up the security context of the
caller, and for code in a DOM window this is the DOM window; as a result,
operations will be performed on the DOM window while attempting to establish
that it's allowed to make this call.

(In reply to comment #4)
> I would have thought simply doing the steps to reproduce would have shown you
> the problem, but since you insist here is the stack:

I would have thought you, of all people, would understand the need for a stack
with an assertion failure.

bsmedberg, thank you for totally failing to say anything consturctive.

Also, this bug is mislabeled. ChatZilla does NOT touch DOM. I will reserve
judement on XPConnect's sanity until a later date after people have stopped
being such idiots.
> I would have thought you, of all people, would understand the need for a stack

Why, when the failure is 100% reproducible so anyone with a debugger can just
get the stack?

> Also, this bug is mislabeled. ChatZilla does NOT touch DOM.

It does, because it runs code on a DOM JS context.  That means that properties
of the JS context might be needed at any time for whatever internal purpose, and
there is no way to avoid that.

I'm not sure what makes you think people are being "idiots" and what's happened
to make you so upset, but feel free to send me personal mail about it if you want...
> Why, when the failure is 100% reproducible so anyone with a debugger can just
> get the stack?

Since when does everyone automatically have a debug build and a debugger
available to them? I happen to be away from my home (never mind my computer) for
a few days, and I don't even know if Mozilla will build on my build box *anyway*
(it has a great knack of totally failing).

And before you ask why I didn't wait until I got back, it's because I wanted to
know how code that very specifically did NOT go near DOM managed to trigger such
an assertion. Obviously, the sheer weirdness of XPConnect was not taken into
account.

> It does, because it runs code on a DOM JS context.  That means that properties
> of the JS context might be needed at any time for whatever internal purpose, and
> there is no way to avoid that.

*XPConnect* touches DOM, not ChatZilla. The fact XPConnect's design is flawed is
a separate issue.

I hope you realise that this makes real threading in JS impossible, too - even
though the API for starting a new thread is scriptable.

As for idiots, that's not you bz. I'm also not upset, just annoyed at the
stupidity of certain people (again, not you) within the Mozilla project. Either
way, they should be able to work it out themselves without me expalining here.
Yeah, I've had a lot of trouble trying to use XPConnect on a background thread
as well.  I tried to execute PAC on a background thread, but I couldn't get
XPConnect to work with me on that.  I eventually gave up.  I think it's best to
keep JS on the main thread.  At least in this case, there's no real benefit to
using a background thread for receiving the DNS lookup result.
What would it take to get the window object to be threadsafe? I suppose probably
the generic element would need a beating to do most of it, if it is not already.

Somewhat related, how does line 608 here work without causing an assertion?

http://lxr.mozilla.org/seamonkey/source/extensions/irc/js/lib/irc.js#602

I'm reasonably sure Necko calls us on its own thread (the stream listener in
connection-xpcom.js calls onStreamDataAvailable), but that call in turn calls a
native function to read the data from the input stream itself. Does Necko not
call stream listeners on its thread as claimed previously, or does reafing from
an input stream not need any security checks?
> What would it take to get the window object to be threadsafe?

probably a lot of work... and it is probably well beyond the scope of the
Mozilla 1.8 branch.


> Somewhat related, how does line 608 here work without causing an assertion?
> 
> http://lxr.mozilla.org/seamonkey/source/extensions/irc/js/lib/irc.js#602
> 
> I'm reasonably sure Necko calls us on its own thread... Does Necko not
> call stream listeners on its thread as claimed previously

not sure where that was claimed.  necko calls stream listeners on the thread
that called asyncOpen, which is always the main application thread.
I wasn't for a minute suggesting any such thing for the 1.8 branch.

I guess the Necko thing must just be something that someone once said, and has 
been passed on from there. I've heard it time and again, though, so Bob only 
knows how it got so commonly known when it is wrong (such is the way of 
documentation-by-word-of-mouth, for lack of anything better).
> The fact XPConnect's design is flawed

XPConnect can't very well avoid touching the execution context; I don't know
what design would allow it to do that, exactly.

> I hope you realise that this makes real threading in JS impossible, too

It's quite possible inside a JS component, which doesn't come anywhere near the
DOM...

> What would it take to get the window object to be threadsafe?

A huge amount of work, if you mean making the DOM threadsafe.  If nothing else,
there are lots of static vars used in gklayout without any sort of locking.

Worse yet, just doing this straight off would be a fairly big performance hit
that I don't think we're willing to take.
people have actively refused to allow that change because threadsafety=perf hit.
There should be no need for XPConnect to touch any objects involved within the 
context of execution - only the context itself - when checking if the context 
has permission X. Anyway, I don't really care who you blame within the 
DOM/Spidermonkey/XPConnect mess - it's imaterial, now that we've established 
that JS threading simply wont work today.

Also, I didn't say DOM, I said window object. From the POV of making 
the "Mozilla Platform" not suck, though, I suppose you would really need to do 
the entire thing.
> only the context itself

The context is the window...  This is nothing to do with XPConnect and
everything to do with the way the DOM sets up its scripting.

Again JS threading works fine.  Simply don't define any of the scripts involved
on a DOM window.
Note: all of the core code *will* be ending up within the scope of a JS XPCOM
Component prior to CZ 1.0, so this will revert back to proper threading at that
point.
Attachment #195729 - Flags: review?(rginda)
fwiw, the exact part of this function that causes this assertion is the calling
of getService, a global function here. that accesses the global object, which is
the window.
Uh, you mean the getService call which does not exist? The stack also shows it
is the access of a method via XPConnect that triggers the assertion.
Comment on attachment 195729 [details] [diff] [review]
Use main thread to work around DOM suckage

r=rginda
Attachment #195729 - Flags: review?(rginda) → review+
Checked in --> FIXED.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
(In reply to comment #18)
> Uh, you mean the getService call which does not exist? The stack also shows it
> is the access of a method via XPConnect that triggers the assertion.

oops, sorry, I suck...
Whiteboard: [cz-0.9.69]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: