Dragana Damjanovic [:dragana] Assignee
	Description • 9 years ago

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

	Dragana Damjanovic [:dragana] Assignee
	Comment 1 • 9 years ago

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

	Dragana Damjanovic [:dragana] Assignee
	Comment 2 • 9 years ago

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

	Patrick McManus [:mcmanus]
	Comment 3 • 9 years ago

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..

	Dragana Damjanovic [:dragana] Assignee
	Comment 4 • 9 years ago

(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.

	Dragana Damjanovic [:dragana] Assignee
	Comment 6 • 9 years ago

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

	Patrick McManus [:mcmanus]
	Comment 7 • 9 years ago

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

	Pulsebot
	Comment 8 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 9 • 9 years ago
bugherder

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