Don't close a stream in an assertion

RESOLVED FIXED in Firefox 50

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox49 wontfix, firefox50 fixed, firefox51 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Because assertions disappear in non-debug builds.
(Assignee)

Comment 1

2 years ago
Created attachment 8787490 [details] [diff] [review]
Don't close a stream in an assertion
Attachment #8787490 - Flags: review?(amarchesini)
Attachment #8787490 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 2

2 years ago
Created attachment 8787519 [details] [diff] [review]
Don't close a stream in an assertion

New version that (a) compiles in opt builds, and (b) doesn't depend on another
patch in my queue, which means it will backport more easily.
(Assignee)

Updated

2 years ago
Attachment #8787490 - Attachment is obsolete: true
(Assignee)

Comment 3

2 years ago
Comment on attachment 8787519 [details] [diff] [review]
Don't close a stream in an assertion

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

Carrying over r+.
Attachment #8787519 - Flags: review+

Comment 5

2 years ago
Pushed by nnethercote@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6246f5ede4a
Don't close a stream in an assertion. r=baku.

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d6246f5ede4a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Comment 7

2 years ago
Comment on attachment 8787519 [details] [diff] [review]
Don't close a stream in an assertion

Approval Request Comment

[Feature/regressing bug #]: bug 994190 introduced the bug.

[User impact if declined]: Files failing to be closed when they should, causing file descriptors to be leaked. It might also cause memory leaks in some cases, but I'm not certain about that.

[Describe test coverage new/current, TreeHerder]: the line of code in question runs in debug builds. It is unintentionally excluded in non-debug builds.

[Risks and why]: Extremely low. It affects a single function call, and only changes behaviour in non-debug builds.

[String/UUID change made/needed]: none.
Attachment #8787519 - Flags: approval-mozilla-beta?
Attachment #8787519 - Flags: approval-mozilla-aurora?
If I understand correctly, this is clearly not a recent regression. As we are super late in the 49 cycle, I would rather see that rides the train with 50
status-firefox49: --- → wontfix
status-firefox50: --- → affected
Attachment #8787519 - Flags: approval-mozilla-beta?
Attachment #8787519 - Flags: approval-mozilla-beta-
Attachment #8787519 - Flags: approval-mozilla-aurora?
Attachment #8787519 - Flags: approval-mozilla-aurora+

Comment 9

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/efc8bd58d7c8
status-firefox50: affected → fixed
You need to log in before you can comment on or make changes to this bug.