1240481 - Limit time for PR_Close calls during shutdown

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Dragana Damjanovic [:dragana]]

If shutdown takes to long, skipp calling PR_Close and just let socket leaks.

Comment 1

[Dragana Damjanovic [:dragana]]

Attachment: bug_1240481_limit_pr_close_calls.patch

I added a pref so we can turn it off if we do not need it.

Comment 2

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c46d384fdba0

Comment 3

[Patrick McManus [:mcmanus]]

Comment on attachment 8708968 [details] [diff] [review]
bug_1240481_limit_pr_close_calls.patch

Review of attachment 8708968 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpref/init/all.js
@@ +1483,5 @@
>  pref("network.sts.max_time_for_events_between_two_polls", 100);
> +
> +// During shutdown we limit PR_Close calls. If time exceeds this pref (in ms)
> +// let sockets just leak.
> +pref("network.sts.max_time_for_pr_close_during_shutdown", 240000);

isn't that number huge? I was expecting something more like ~5000

::: netwerk/base/nsSocketTransport2.cpp
@@ +1754,5 @@
>      if (--mFDref == 0) {
> +        if (gIOService->IsNetTearingDown() &&
> +            ((PR_IntervalNow() - gIOService->NetTearingDownStarted()) >
> +             gSocketTransportService->MaxTimeForPrClosePref())) {
> +          mFD = nullptr;

socket_log("intentional leak")

@@ +1755,5 @@
> +        if (gIOService->IsNetTearingDown() &&
> +            ((PR_IntervalNow() - gIOService->NetTearingDownStarted()) >
> +             gSocketTransportService->MaxTimeForPrClosePref())) {
> +          mFD = nullptr;
> +        }

else if ?

::: netwerk/base/nsSocketTransportService2.cpp
@@ +1215,5 @@
> +        int32_t maxTimeForPrClosePref;
> +        rv = tmpPrefService->GetIntPref(MAX_TIME_FOR_PR_CLOSE_DURING_SHUTDOWN,
> +                                        &maxTimeForPrClosePref);
> +        if (NS_SUCCEEDED(rv) && maxTimeForPrClosePref >=0) {
> +            mMaxTimeForPrClosePref = maxTimeForPrClosePref;

I don't think you're supposed to assume PRInterval is stored in milliseconds.. there is a PER macro for that..

Comment 4

[Dragana Damjanovic [:dragana]]

(In reply to Patrick McManus [:mcmanus] from comment #3)
> Comment on attachment 8708968 [details] [diff] [review]
> bug_1240481_limit_pr_close_calls.patch
> 
> Review of attachment 8708968 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: modules/libpref/init/all.js
> @@ +1483,5 @@
> >  pref("network.sts.max_time_for_events_between_two_polls", 100);
> > +
> > +// During shutdown we limit PR_Close calls. If time exceeds this pref (in ms)
> > +// let sockets just leak.
> > +pref("network.sts.max_time_for_pr_close_during_shutdown", 240000);
> 
> isn't that number huge? I was expecting something more like ~5000

I wanted this to be used only in extreme cases when when we really exceed time. So it is set to 4min, we will crash at 5min. This is an arbitrary number so 5s is ok too, since we are not doing much harm if we leak sockets at shutdown. I will set it to 5.

Comment 5

[Dragana Damjanovic [:dragana]]

Attachment: bug_1240481_limit_pr_close_calls.patch

Comment 6

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=36539ff8f60c

Comment 7

[Patrick McManus [:mcmanus]]

its not impossible to think of this causing intermittent orange leaks.. just something to keep in mind

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/66bce0b7c669

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/66bce0b7c669