1053447 - Re-enable background finalization of ProxyObjects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eric Faust [:efaust]]

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

Comment 1

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/79f6d0449fd1

Comment 2

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/79f6d0449fd1

Comment 3

[Eric Faust [:efaust]]

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

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9b8c0def997a

Comment 5

[Lawrence Mandel [:lmandel] (use needinfo)]

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

Comment 6

[Eric Faust [:efaust]]

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

Comment 7

[Lawrence Mandel [:lmandel] (use needinfo)]

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.