Re-enable background finalization of ProxyObjects

RESOLVED FIXED in Firefox 33

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: efaust, Assigned: efaust)

Tracking

unspecified
mozilla34
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox32 wontfix, firefox33 fixed, firefox34 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
Created attachment 8472596 [details] [diff] [review]
Patch

As part of testing for a b2g blocker, I misguidedly landed a patch to disable background finalization of proxy objects. Let's undo that.

We've talked about this on IRC already, but I made a new bug and posted the patch formally, so we could get it uplifted as needed.
Attachment #8472596 - Flags: review?(terrence)
Attachment #8472596 - Flags: review?(terrence) → review+

Comment 2

4 years ago
https://hg.mozilla.org/mozilla-central/rev/79f6d0449fd1
Assignee: nobody → efaustbmo
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
(Assignee)

Comment 3

4 years ago
Comment on attachment 8472596 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: Testing in bug 1008791
[User impact if declined]: Some GC performance
[Describe test coverage new/current, TBPL]: landed comfortably on trunk
[Risks and why]: Little. This worked before, and we disabled it temporarily. We're just re-enabling and old feature.
[String/UUID change made/needed]: None
Attachment #8472596 - Flags: approval-mozilla-beta?
Attachment #8472596 - Flags: approval-mozilla-aurora?
status-firefox32: --- → affected
status-firefox33: --- → affected
status-firefox34: --- → fixed
Attachment #8472596 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Eric Faust [:efaust] from comment #0)
> Created attachment 8472596 [details] [diff] [review]
> Patch
> 
> As part of testing for a b2g blocker, I misguidedly landed a patch to
> disable background finalization of proxy objects. Let's undo that.
> 
> We've talked about this on IRC already, but I made a new bug and posted the
> patch formally, so we could get it uplifted as needed.

This feature was disabled on May 29, which means that 32 went through the entire Aurora and Beta cycles to this point with this disabled. Although a very small code change, is it worth the risk of reeabling this feature at this point and shipping something that was not tested by the prerelease user base during the 32 cycle?
Flags: needinfo?(efaustbmo)
(Assignee)

Comment 6

4 years ago
(In reply to Lawrence Mandel [:lmandel] from comment #5)
> (In reply to Eric Faust [:efaust] from comment #0)
> > Created attachment 8472596 [details] [diff] [review]
> > Patch
> > 
> > As part of testing for a b2g blocker, I misguidedly landed a patch to
> > disable background finalization of proxy objects. Let's undo that.
> > 
> > We've talked about this on IRC already, but I made a new bug and posted the
> > patch formally, so we could get it uplifted as needed.
> 
> This feature was disabled on May 29, which means that 32 went through the
> entire Aurora and Beta cycles to this point with this disabled. Although a
> very small code change, is it worth the risk of reeabling this feature at
> this point and shipping something that was not tested by the prerelease user
> base during the 32 cycle?

The cost of not taking the patch is relatively low (some perf hit, but not even that much), and I agree that applying it may introduce new crashes (unlikely, but possible). If we feel like 32 is too close to release to take this, that makes sense. It's a thing that should bake for a bit under tests, because the crashes (in the worst case, if there were any) that it caused would be intermittent GC-related failures.

I am OK with shipping a release without this patch applied. It will not be a disaster.
Flags: needinfo?(efaustbmo)
Thanks Eric. Given the number of changes that are going in on beta, I'd like to play it safe in this case. I'm marking Firefox 32 as won't fix. This fix will be in Firefox 33.
status-firefox32: affected → wontfix
Attachment #8472596 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.