In bug 1125391 comment 10 we had to turn off the bug 931668 optimisations on mozilla-b2g37_v2_2 due to crashes in test_sort_contacts.py. I've managed to reproduce locally, and after turning on the more extensive assertions in the nsStyleContext destructor, we get this while running test_sort_contacts.py: style struct 0x7f04178c7e00 found on style context 7f0417185650 in app://communications.gaiamobile.org/contacts/index.html Assertion failure: false (destroying Font style struct still present in style context tree), at ./nsStyleStructList.h:44 With this bug's patch applied and without the bug 1127198 patches, we don't get the assertion or crash, so there is probably an issue with the latter.

(In reply to Cameron McCormack (:heycam) from comment #0) > In bug 1125391 comment 10 we had to turn off the bug 931668 optimisations on > mozilla-b2g37_v2_2 due to crashes in test_sort_contacts.py. Just to be clear, bug 1125391 was backed out from Aurora, so this affects all Gecko 37, i.e. Beta as well.

[Tracking Requested - why for this release]: So that we're aware if this doesn't get fix that the perf regression will continue on Firefox 37. (We already had to disable the optimisations on Firefox 36.)

The issue is that the coalescing added in bug 1127198 part 4 isn't valid. What that coalescing tries to do is to avoid having both a style context and its parent in mContextsToClear; if the child is the last element in the array and we're about to append its parent, then we replace the child with its parent in the array. This is done under the assumption that the work done by calling ClearCachedInheritedStyleDataOnDescendants on the parent subsumes that done by calling it on the child. This is not correct, however, when ClearCachedInheritedStyleDataOnDescendants stops traversing down the tree because it detects that a new "owned" style struct is encountered. (ClearCachedInheritedStyleDataOnDescendants stops the traversal because the original struct value that we are looking for can no longer appear in those descendant style contexts, once we encounter a new "owned" struct.) The reason this doesn't reproduce on Firefox 38 is because of bug 1067755, which prevents the style contexts involved from hanging around.

This document asserts on mozilla-beta if we turn the bug 931668 optimisations back on.

Let's just remove the coalescing.

Bug 1125391 is already tracked. As this bug doesn't currently affect 37 but rather blocks uplift of bug 1125391, I don't think we need to track this bug specifically. That doesn't mean that we shouldn't consider uplifting a fix if we have a good one and are ready to do so.

Comment on attachment 8569091 [details] [diff] [review] patch Did you test that the crashtest crashes in the harness without the patch? If so, r=dbaron.

It does: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2d396f73f45

Comment on attachment 8569091 [details] [diff] [review] patch Please consider this an approval request to re-land bug 1125391's patch, re-enabling the optimizations, too, for mozilla-beta/b2g37. Approval Request Comment [Feature/regressing bug #]: bug 1125391 [User impact if declined]: performance regression [Describe test coverage new/current, TreeHerder]: this bug's patch (the bug fix for the optimizations) just landed on inbound; the underlying optimizations were already enabled on aurora/central [Risks and why]: this bug fix is relatively straightforward; the underlying optimizations have been enabled for a couple of months, on different branches [String/UUID change made/needed]: N/A

I've also tested locally, on b2g37/beta, that the bug fix solves the problem that prevented the merge b2g37 merge originally.

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=7189515&repo=mozilla-inbound

waiting for branch approval till it lands on m-c again.

ni heycam as this was backed out.

I think we can just update the assertion count on that test too as it's basically the same as the one I updated the assertion count for in the patch.

(In reply to Cameron McCormack (:heycam) from comment #16) > I think we can just update the assertion count on that test too as it's > basically the same as the one I updated the assertion count for in the patch. Er, not the one in this patch, but the one Ryan mentioned in bug 1125391 that I'll be adjusting when I land on branches.

Trying again: https://hg.mozilla.org/integration/mozilla-inbound/rev/8da6897c4aa1

Comment on attachment 8569091 [details] [diff] [review] patch Looks like this fix has stuck this time. Let's get this into Beta 3. Beta+ Aurora+

Comment on attachment 8569091 [details] [diff] [review] patch Didn't need this ? as it'll just get merged over from beta.

When uplifting, in addition to the assertion bump in this bug's patch, please bump layout/reftests/printing/1108104.html to 4 assertions. Unfortunately I've lost track of whether the try run I am looking at is for aurora or beta, so I'm not certain which branch needed this; I suspect just beta. I will be around for the next 8 hours or so to watch the trees to see if it's aurora too and can land the assertion annotation there too if necessary.

Oh, I worked it out. It is needed on both aurora and beta. Thanks.

Beta: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b8e77084fce Aurora: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4b630c41986

Note that this bug missed 37 Beta 3. It will be included in 37 Beta 4.