Don't close a stream in an assertion

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 wontfix, firefox50 fixed, firefox51 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

3 years ago
Because assertions disappear in non-debug builds.
Assignee

Comment 1

3 years ago
Attachment #8787490 - Flags: review?(amarchesini)
Attachment #8787490 - Flags: review?(amarchesini) → review+
Assignee

Comment 2

3 years ago
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

3 years ago
Attachment #8787490 - Attachment is obsolete: true
Assignee

Comment 3

3 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

3 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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d6246f5ede4a
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Assignee

Comment 7

3 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
Attachment #8787519 - Flags: approval-mozilla-beta?
Attachment #8787519 - Flags: approval-mozilla-beta-
Attachment #8787519 - Flags: approval-mozilla-aurora?
Attachment #8787519 - Flags: approval-mozilla-aurora+
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.