Closed Bug 1240481 Opened 4 years ago Closed 4 years ago

Limit time for PR_Close calls during shutdown

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: dragana, Assigned: dragana)

References

Details

Attachments

(1 file, 1 obsolete file)

If shutdown takes to long, skipp calling PR_Close and just let socket leaks.
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
I added a pref so we can turn it off if we do not need it.
Attachment #8708968 - Flags: review?(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..
Attachment #8708968 - Flags: review?(mcmanus)
(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.
Attachment #8708968 - Attachment is obsolete: true
Attachment #8709073 - Flags: review?(mcmanus)
Attachment #8709073 - Flags: review?(mcmanus) → review+
its not impossible to think of this causing intermittent orange leaks.. just something to keep in mind
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/66bce0b7c669
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.