181230 - profile-change-net-teardown must work

Status: VERIFIED FIXED

(Core :: Networking, defect, P2)

Description

[Kai Engert [:KaiE:]]

Working on bugs 177260 and 97622 I ran into an additional problem.
I thought I had a good workaround, but the workaround requires direct
manipulation of the lower NSPR data structures from outside the socket transport
thread, and that is possibly dangerous.

If, at the time we need to shut down PSM/NSS in order to switch profiles, there
are still sockets alive, we have a real problem.

Because the SSL library is layering itself on the file descriptor stack, the
resources that SSL is holding into NSS can not be freed until the socket is closed.


Actual behaviour:
After the profile-change-net-teardown phase is over, there are still open sockets.

Requested behaviour:
During the profile-change-net-teardown phase, all open sockets must be closed
synchronously.

Comment 1

[Darin Fisher]

Attachment: test patch

this patch ensures that any active connections will not be added as idle
connections after the net-teardown event fires.

Comment 2

[Darin Fisher]

Attachment: AIM conversation with kai

kai says the "test patch" doesn't help.

Comment 3

[Kai Engert [:KaiE:]]

Darin, in your test patch you try to modificate the http handler. I would have
expected that the problem lies somewhere else. I see that mail is also not
behaving correctly and stops us from switching profiles, because SSL sockets
have not been closed.

Are you able to tear down all sockets the hard way in the socket transport code,
regardless where they come from?

Comment 4

[Darin Fisher]

kai: my assumption was that socket transport shutdown was happening, but that
some connections were becoming IDLE (on a background thread) at the same time
that we were processing the net-teardown event.  i think my patch closes this
hole, but obviously there is something else going on as well.  i'm hoping to
spend time on this today.

Comment 5

[Darin Fisher]

Attachment: v1 patch

this patch fixes the "going offline while security dialog is visible" problem.

Comment 6

[Darin Fisher]

kai: can you please give this latest patch a try and see if it fixes all of your
testcases?  thx!

Comment 7

[Kai Engert [:KaiE:]]

Sorry, this patch does not change the behaviour, I still see the same problems.

I would have assumed there is only one approach than can help us:
When we receive the call to SetOffline, go to the socket transport code,
synchronously iterate over the list of all sockets known by it, call close on
each, and do not return from the notification until that is done.

But that is probably dangerous, because other code has still references to the
socket.

So I suggest, instead of closing the files, which invalidates the file
descriptors and makes other components crash: Use the hack that I proposed in
bug 177260, but which you found to be unsafe when done from outside the socket
thread.

So the new approach might be:

- SetOffline received
- iterate over all sockets
- for each socket, do the following hack:
  - use PR_NewTCPSocket to create a new unconnected socket
  - swap all the contents of the socket filedescriptor structure
    between the old and the hack socket
  - close the fd that is now no longer referenced by others

Comment 8

[Darin Fisher]

> I would have assumed there is only one approach than can help us:
> When we receive the call to SetOffline, go to the socket transport code,
> synchronously iterate over the list of all sockets known by it, call close on
> each, and do not return from the notification until that is done.

that is what the code tries to do.  it cancels all of the socket transport
requests, which must be processed on the socket transport thread.  the thread
which shutdown the socket transport service (the main thread) must wait until
the socket transport thread exits (it does a join).  this makes the call to
shutdown the socket transport synchronous.

kai: what i need is a testcase.  i used the testcase of going offline while the
security dialog is showing to arrive at this patch.  is that what you were
testing?  before my patch i noticed PR_Close being called after the security
dialog is closed, but with my patch PR_Close is called right away.  please
describe the steps you used to repro the problem.  thx!

Comment 9

[Kai Engert [:KaiE:]]

The testcase is:

- add printf statements as the first line to nsNSSIOLayerClose and
nsSSLIOLayerConnect in file nsNSSIOLayer.cpp

- compile in optimized mode (I tested on Linux)

- open two browser windows

- in one window, open https://meine.db24.de - wait for the security window and
leave it open

- watch the console, you'll see the output from the open function

- in the other window, click the statusbar offline icon


Expected behaviour: The SSL close function should have been reached as soon as
you clicked the offline icon. As a result, the console log should show the
printf output from the close function.

Actual behavior: No output on the console.



Another testcase:
- Configure mail to use IMAP / SSL.
- Configure edit/privacy/certificates to "ask every time"
- Open a mail and a browser window
- in one window, connect to the server
- should it prompt to select a certificate, cancel
- you should see the prompt to enter your mail password
- with the password prompt dialog open, switch to the browser window
- click the offline icon.

Expected: close is reached immediately.
Actual: Nothing happens.

Comment 10

[Kai Engert [:KaiE:]]

> i used the testcase of going offline while the
> security dialog is showing to arrive at this patch.  is that what you were
> testing?  before my patch i noticed PR_Close being called after the security
> dialog is closed, but with my patch PR_Close is called right away

Hmm, I can not confirm that, on my Linux system, close is not reached right
away. It is not reached until I confirm the security warning dialog.

Comment 11

[Darin Fisher]

Attachment: v1.1 patch

OK, this patch is similar to the other patch in that it only attempts to fix
the problem for HTTPS.

Comment 12

[Darin Fisher]

Attachment: v1.2 patch

this one should really do the trick for HTTPS.

Comment 13

[Kai Engert [:KaiE:]]

This latest patch does indeed work for the https case. (Although it does not yet
work for mail/ssl, as discussed).

Comment 14

[Michael Buckland]

Discussed in edt.  Plussing because we need this for profile sharing.

Comment 15

[Kai Engert [:KaiE:]]

Darin, is your intention to implement the general solution (which would include
tearing down mail ssl sockets) within the scope of this bug?

Comment 16

[Darin Fisher]

Kai:

yes, it is.  but i want to get this HTTP patch in because i think it is anyways
a good idea, and it might even fix a topcrash.

Comment 17

[Kai Engert [:KaiE:]]

Ok, thanks!

Comment 18

[Darin Fisher]

Comment on attachment 107386 [details] [diff] [review]
v1.2 patch

seeking drivers approval to check this into the trunk for moz 1.3 alpha.  this
patch not only helps fix this bug for HTTPS connections, but it might also
solve topcrash bug 179391.  i think this patch has moderate risk.

Comment 19

[Asa Dotzler [:asa]]

Comment on attachment 107386 [details] [diff] [review]
v1.2 patch

a=asa for checkin to 1.3a

Comment 20

[Darin Fisher]

landed patch on trunk.  next, i'll look into how to fix this in general. 
hopefully this patch will help with the topcrash shutdown bug.

Comment 21

[Kai Engert [:KaiE:]]

Nominating to block 1.3b, because not having this fix results in crash bug 182803. 

Comment 22

[Kai Engert [:KaiE:]]

Comment on attachment 107386 [details] [diff] [review]
v1.2 patch

In the past, when people landed patches on the trunk, but decided to keep the
bug open, I saw they marked the landed patches as obsolete to minimize
potential confusion. I think that's a good idea. Marking obsolete.

Comment 23

[Darin Fisher]

should be fixed by my patch for bug 176919.

Comment 24

[Darin Fisher]

FIXED with patch from bug 176919.

Comment 25

[Ere Maijala (slow)]

*** Bug 188558 has been marked as a duplicate of this bug. ***

Comment 26

[benc]

V/fixed.

This has worked for some time. I haven't tested it with a set of unit cases, since profile changing is available only in seamonkey UI. 

I'd like to do this in the future as an high-level integration test, maybe as an extension of the networking-core functional test I wrote (which pokes a lot of network protocols at once). Bother me if you care.