Jesse Ruderman Reporter
	Description • 18 years ago

Steps to reproduce:
1. Load https://www.paypal.com/ or https://bugzilla.mozilla.org/ with Firefox (debug trunk build on Intel Mac).

###!!! ASSERTION: nsSecureBrowserUIImpl not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file /Users/jruderman/trunk/mozilla/security/manager/boot/src/nsSecureBrowserUIImpl.cpp, line 173

This regressed within the last few days.

	Peter Van der Beken [:peterv]
	Comment 1 • 18 years ago

I think this is a regression from bug 107491.

	Peter Van der Beken [:peterv]
	Comment 2 • 18 years ago

Here's the relevant part of the stack when I first hit this:

#7  0x02b077a8 in nsSecureBrowserUIImpl::AddRef (this=0x2b6127e0) at mozilla/security/manager/boot/src/nsSecureBrowserUIImpl.cpp:172
#8  0x0f961b9c in ns_if_addref<nsISecureBrowserUI*> (expr=0x2b6127e0) at ../../dist/include/xpcom/nsISupportsUtils.h:114
#9  0x0f908ffa in nsDocShell::GetSecurityUI (this=0x2b4dc530, aSecurityUI=0xb009d9a0) at mozilla/docshell/base/nsDocShell.cpp:1479
#10 0x2ffd9894 in nsNSSSocketInfo::SetNotificationCallbacks (this=0x2c9d8660, aCallbacks=0x2c9b0cb4) at mozilla/security/manager/ssl/src/nsNSSIOLayer.cpp:336
#11 0x0bef9d6b in nsSocketTransport::BuildSocket (this=0x2c9d4d10, fd=@0xb009dc5c, proxyTransparent=@0xb009dc58, usingSSL=@0xb009dc54) at mozilla/netwerk/base/src/nsSocketTransport2.cpp:1030
#12 0x0befa74e in nsSocketTransport::InitiateSocket (this=0x2c9d4d10) at mozilla/netwerk/base/src/nsSocketTransport2.cpp:1098
#13 0x0befbbf3 in nsSocketTransport::OnSocketEvent (this=0x2c9d4d10, type=1, status=0, param=0x2c9dd990) at mozilla/netwerk/base/src/nsSocketTransport2.cpp:1414
#14 0x0bfbb874 in nsSocketEvent::Run (this=0x2c9d7b80) at mozilla/netwerk/base/src/nsSocketTransport2.cpp:97
#15 0x01c1acf8 in nsThread::ProcessNextEvent (this=0x2a2f460, mayWait=1, result=0xb009dd9c) at mozilla/xpcom/threads/nsThread.cpp:482
#16 0x01bc370e in NS_ProcessNextEvent_P (thread=0x2a2f460, mayWait=1) at nsThreadUtils.cpp:225
#17 0x0befe55f in nsSocketTransportService::Run (this=0x3060400) at mozilla/netwerk/base/src/nsSocketTransportService2.cpp:525
#18 0x01c1acf8 in nsThread::ProcessNextEvent (this=0x2a2f460, mayWait=1, result=0xb009deac) at mozilla/xpcom/threads/nsThread.cpp:482

	Kai Engert (:KaiE:) Assignee
	Comment 3 • 18 years ago

Peter, thanks for the stack, because I do not get this assertion on Linux.

We probably need to proxy the call to GetSecurityUI?

	Kai Engert (:KaiE:) Assignee
	Comment 4 • 18 years ago

Peter or Jesse, if you're easily able to, could you please help me and test this patch on Mac?
Thanks!

	Kai Engert (:KaiE:) Assignee
	Comment 5 • 18 years ago

Comment on attachment 255786 [details] [diff] [review]
Patch v1

no, this doesn't work. and I'm able to reproduce now. I think I have a better solution, testing currently.

	Kai Engert (:KaiE:) Assignee
	Comment 6 • 18 years ago

I'm attaching a patch that will solve this issue.

In case you are surprised by this patch and would like to understand the background, read on.

The existing implementation ob that object is not threadsafe. It doesn't need to be, all calls to this object happen from a single thread.

With the new patch from bug 107491, I introduced a "test for existence", which happens to get executed on a different thread. As Peter noted correctly to me on IRC, in order to avoid doing reference counting on the wrong thread, this bug might get solved by a new nsIDocShell::HasSecurityUI function.

However, when I started thinking about it, whether this is a reasonable addition to nsIDocShell, I concluded we should name it differently. It should express what the flag is supposed to be used for.

The question it shall answer is: Is the context of this nsIDocShell able to display crypto errors as an error page?

Because of that I'm proposing to use:
  readonly attribute boolean useCryptoErrorPages;

This is what the attached patch does.

But now you might say, we already have
   attribute boolean useErrorPages;
so why do we need another attribute for the crypto errors?

Well, the attribute useErrorPages does not reflect reality.
I learned that Thunderbird always has this set to TRUE, although Thunderbird never displays errors as an error page! I learned this was done as a hack in order to suppress some error messages. (See cvs blame for file all-thunderbird.js line error_pages = true)

So, I want a general solution.
I want the crypto networking code to clearly find out, whether it should bring up a prompt on its own, or whether the calling application code wants to report the error.

I would like to avoid to introduce application specific error reporting, like adding new code to Thunderbird's error reporting. Because that approach would mean, I would have to do that for every other application... say Sunbird etc.

So the proposal is:
- introduce the new nsIDocShell property, which will avoid the assertion reported in this bug
- the current implementation will return "true" when it is a browser window which is able to show an error page
- for all other windows the flag will be "false" and the crypto code will bring up the error dialog on its own (as it is doing it today! no change here)
- if an application ever wants to decide for itself, to display these errors in some other way, it can easily do so, by making sure that useCryptoErrorPages returns "true" and by using code similar as in nsDocShell::DisplayLoadError.

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 18 years ago

For what it's worth, docshell's claims of being threadsafe are a base lie...  we've been considering making its refcounting non-threadsafe.

Is there any way this code could run on the main thread?

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 9 • 18 years ago

> I learned that Thunderbird always has this set to TRUE, although Thunderbird
> never displays errors as an error page!

I've definitely seen error pages in mailnews in SeaMonkey. Is this really different in thunderbird?

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 10 • 18 years ago

Or put differently, what protocol did you test with?

	David Baron :dbaron:
	Comment 11 • 18 years ago

I also see this on Linux; the stack is exactly the same as in comment 2.

	David Baron :dbaron:
	Comment 12 • 18 years ago

Could it be that nsSocketTransport::BuildSocket shouldn't be calling SetNotificationCallbacks on this thread?

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 17 years ago

Sicking, apparently, people do actually touch nsIDocShell from off-thread.  :(  We probably need to whip this code into shape before we can cycle-collect docshells.

See also bug 243591.

Ideally, we would do some combination of that and the patch in this bug, modulo biesi's concerns about this whole approach.

Speaking of which, did we ever sort out what this code _should_ be doing?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 17 • 17 years ago

Yes, please please please don't access docshells (any interface of it) from any other thread than the main thread. It's going to block some leak work that we're going to have to do very soon.

	David Baron :dbaron:
	Comment 18 • 17 years ago

Could somebody either get a fix for this checked in (maybe attachment 264887 [details] [diff] [review]?) or get bug 107491 backed out in the next week?

	Nelson Bolyard (seldom reads bugmail)
	Comment 19 • 17 years ago

Isn't this all yet another consequence of the checkin for Bug 326273 ?

	David Baron :dbaron:
	Comment 20 • 17 years ago

No.  It was a regression as described in comment 1 and comment 6.

	Kai Engert (:KaiE:) Assignee
	Comment 21 • 17 years ago

(In reply to comment #9)
> > I learned that Thunderbird always has this set to TRUE, although Thunderbird
> > never displays errors as an error page!
> 
> I've definitely seen error pages in mailnews in SeaMonkey. Is this really
> different in thunderbird?


Configure a bad SMTP server.
Send an email.
No error page.
You get an error dialog.


Mailnews still uses dialogs to report errors during network connection, at least in all the scenarios that I experienced.

IMHO we can not base our decision on the existing flag useErrorPages, because Mailnews incorrectly sets it to true.

I call it "incorrectly", because it's not complete.

If I base my error reporting on the existing flag useErrorPages, and a crypto error occurs during an SSL connection, no error message will be shown.

In order to be make sure this shared code will work equally in all apps, in order to be independent of the hacks used for mailnews code, I proposed to introduce the seperate flag.

	Kai Engert (:KaiE:) Assignee
	Comment 22 • 17 years ago

Comment on attachment 256024 [details] [diff] [review]
Patch v2

Marking as obsolete because this directly accesses the DocShell on the wrong thread.

	Kai Engert (:KaiE:) Assignee
	Comment 23 • 17 years ago

Comment on attachment 264887 [details] [diff] [review]
Cheap and cheerful fix for those waiting for a real solution ;-)

Boris, Jonas, in this patch Neil proposes to proxy the call to docshell.

Is this an acceptable solution to you?

	Kai Engert (:KaiE:) Assignee
	Comment 24 • 17 years ago

From IRC channel #devlopers:

<kaie> in bug 370875 sicking said, we shouldn't access any DocShell interfaces from any thread other than the main thread. are proxied calls to docshell ok?
<sicking> kaie: yes, since then you're accessing it on the main thread


I conclude Neil's patch is acceptable.

	Kai Engert (:KaiE:) Assignee
	Comment 25 • 17 years ago

Comment on attachment 264887 [details] [diff] [review]
Cheap and cheerful fix for those waiting for a real solution ;-)

r=kengert

I tested this patch, I no longer get the assertions.

Thanks Neil!

	Kai Engert (:KaiE:) Assignee
	Comment 26 • 17 years ago

I checked the patch in to trunk, please comment if you think this should get changed in a different way. Marking fixed.

	Boris Zbarsky [:bzbarsky]
	Comment 27 • 17 years ago

The patch is fine for avoiding the assert here, but you're still doing the addref/release of the docshell off the main thread, which doesn't address the issue sicking was having in comment 17.  You can test that easily by making the docshell addref/release non-threadsafe and watching the resulting asserts.

That should probably be a separate bug, however.  And that can be solved by proxying the GetInterface call too, I guess.  But really, I think that the callbacks object should do its own proxying and we should do that in necko.  Or something.