Bugzilla

Assignee

Comment 3

•

7 years ago

This is <marquee> XBL scripts running. This looks a lot like bug 1371259...

Comment 4

•

7 years ago

Kinda does, but that fix was backported to ESR52...  I'll take a look into what's going on here.

Comment 5

•

7 years ago

OK.  This is NOT the same thing as bug 1371259.  The dead thing is not the computed style object; it's the presshell.

In particular, when returning from GetStyleContextForElement we drop the ref to the presshell, and that kills mPresShell.  Why, I'm not sure yet; we have comments about how this is supposed to be safe because we flushed up front.

Comment 6

•

7 years ago

Alright, so here's what happens, at least on 52.  We land in nsComputedDOMStyle::UpdateCurrentStyleSources.  aNeedsLayoutFlush is true.  So we go ahead and flush layout, fine so far.  Then we set mPresShell to document->GetShell().

Then we discover that we need to actually resolve style, so we call nsComputedDOMStyle::GetStyleContextForElement.  This does a style flush, which ought to be a no-op, because we already flushed.  But it's not.  Instead, we get this stack:

#43 0x00007efe3bac5be3 in PresShell::FireResizeEvent() (this=0x268d66630400) at /home/bzbarsky/mozilla/esr52/mozilla/layout/base/nsPresShell.cpp:2038
#44 0x00007efe3bacc61b in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) (this=0x268d66630400, aFlush=...)
    at /home/bzbarsky/mozilla/esr52/mozilla/layout/base/nsPresShell.cpp:4122
#45 0x00007efe3bacc35d in PresShell::FlushPendingNotifications(mozFlushType) (this=0x268d66630400, aType=Flush_Style)
    at /home/bzbarsky/mozilla/esr52/mozilla/layout/base/nsPresShell.cpp:4054
#46 0x00007efe3b8c3dc2 in nsComputedDOMStyle::GetStyleContextForElement(mozilla::dom::Element*, nsIAtom*, nsIPresShell*, nsComputedDOMStyle::StyleType) (aElement=0x7efe449be1f0, aPseudo=0x0, aPresShell=0x268d66630400, aStyleType=nsComputedDOMStyle::eAll)
    at /home/bzbarsky/mozilla/esr52/mozilla/layout/style/nsComputedDOMStyle.cpp:422

and then a resize event runs, we run script, and all bets are off; by the time all that is done mPresShell is dead.  Then we use it, because we think the second flush has to be a no-op.

Bug 1370072 comment 3 basically describes the horrible thing that resize events do.  I went ahead and filed bug 1407031 on that.

Anyway, for purposes of this bug, we should probably just reget mShell, and probably mOuterFrame/mInnerFrame, after the nsComputedDOMStyle::GetStyleContextForElement call...  Cameron, does that make sense?

I expect all branches are affected, though tip may be non-stylo-only.

tracking-firefox56: --- → ?

tracking-firefox57: --- → ?

tracking-firefox58: --- → ?

tracking-firefox-esr52: --- → ?

Flags: needinfo?(cam)

Liz Henry (:lizzard) (relman/hg->git project)

Comment 7

•

7 years ago

Alernately, we could hold a strong ref to mPresShell or equivalent and bail out of it gets destroyed in the nsIPresShell::IsDestroying sense...

Comment 8

•

7 years ago

Tracking across all current versions (though it would be good to have confirmation of which versions are definitely affected)

status-firefox56: --- → ?

status-firefox57: --- → ?

status-firefox58: --- → ?

status-firefox-esr52: --- → affected

tracking-firefox56: ? → +

tracking-firefox57: ? → +

tracking-firefox58: ? → +

Cameron McCormack (:heycam)

Comment 9

•

7 years ago

Even if the pres shell is not IsDestroying(), the mInnerFrame / mOuterFrame pointers could be out of date, is that right?  That's kind of annoying.

What if we use GetStyleContextForElementNoFlush instead?  Is it a big deal if the styles we return don't take into account whatever the resize event will do?  (I'm guessing no?)

Flags: needinfo?(cam) → needinfo?(bzbarsky)

Comment 10

•

7 years ago

Oh, good catch on the frame pointers being out of date.

> What if we use GetStyleContextForElementNoFlush instead?

The biggest problem is that there are potentially two presshells involved: the one for mDocument and the one returned by GetPresShellForContent().  We're flushing the former up front.  In GetStyleContext, we might flush the latter.  The whole thing is a mess.  :(

I guess the simple thing to do would be to flush both up front if they're different.

Flags: needinfo?(bzbarsky)

Andrew McCreight [:mccr8]

Comment 11

•

7 years ago

This testcase crashes for me on trunk with or without Stylo enabled.

Regression range:
INFO: Last good revision: 4d63dde701b47b8661ab7990f197b6b60e543839 (2016-05-27)
INFO: First bad revision: ea15028498ed95677844fb7f30be5efcaf8b2621 (2016-05-28)
INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4d63dde701b47b8661ab7990f197b6b60e543839&tochange=ea15028498ed95677844fb7f30be5efcaf8b2621

Has Regression Range: --- → yes

status-firefox56: ? → wontfix

status-firefox57: ? → affected

status-firefox58: ? → affected

Version: 52 Branch → 49 Branch

Jet Villegas (inactive)

Updated

•

7 years ago

Priority: -- → P2

Jet Villegas (inactive)

Comment 12

•

7 years ago

(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)
> This testcase crashes for me on trunk with or without Stylo enabled.
> 
> Regression range:
> INFO: Last good revision: 4d63dde701b47b8661ab7990f197b6b60e543839
> (2016-05-27)
> INFO: First bad revision: ea15028498ed95677844fb7f30be5efcaf8b2621
> (2016-05-28)
> INFO: Pushlog:
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=4d63dde701b47b8661ab7990f197b6b60e543839&tochange=ea15
> 028498ed95677844fb7f30be5efcaf8b2621

Blocks: 881832

Comment 13

•

7 years ago

(in case somebody doesn't follow the dependency chain all of the way through, bug 881832 was also a fix for a timing attack in bug 928187)

Updated

•

7 years ago

Blocks: 1404589

Matt Woodrow (:mattwoodrow)

Updated

•

7 years ago

Assignee: nobody → matt.woodrow

Flags: sec-bounty?

Keywords: testcase

Comment 14

•

7 years ago

Boris, were you going to write a patch for this? This really isn't code I know well.

Flags: needinfo?(bzbarsky)

Assignee

Comment 15

•

7 years ago

Attached patch Like this — Details — Splinter Review

I haven done all the ASAN setup, but this is how it should look like...

This prevents the shell from dying by not flushing it again (triggering the resize events and so on).

Flags: needinfo?(bzbarsky)

Attachment #8920746 - Flags: review?(bzbarsky)

Assignee

Comment 16

•

7 years ago

Attached patch With a bit less action at a distance. (obsolete) — Details — Splinter Review

Just a bit easier to reason about, I think, and also a bit less risky, keeping the flush only when necessary.

Attachment #8920746 - Attachment is obsolete: true

Attachment #8920746 - Flags: review?(bzbarsky)

Attachment #8920748 - Flags: review?(bzbarsky)

Assignee

Comment 17

•

7 years ago

Err, and I just stuck one extra FIXME that shouldn't appear in the final patch, of course. In any case I've verified this fixes the crash.

Comment 18

•

7 years ago

Comment on attachment 8920748 [details] [diff] [review]
With a bit less action at a distance.

So with this patch, we could have a non-null mInner/OuterFrame but MustReresolveStyle() testing true, and then we end up reaching this code and ending up with dead mInner/OuterFrame after it runs, right?

Seems like we could hit this case asking for computed width from document A on an element that is rendered under first-line in document B, with a pending restyle that will blow away the relevant frames.

Attachment #8920748 - Flags: review?(bzbarsky) → review-

Assignee

Comment 19

•

7 years ago

Comment on attachment 8920746 [details] [diff] [review]
Like this

You're totally right Boris. Seems like the first patch should do it then.

Attachment #8920746 - Attachment is obsolete: false

Attachment #8920746 - Flags: review?(bzbarsky)

Assignee

Updated

•

7 years ago

Attachment #8920748 - Attachment is obsolete: true

Comment 20

•

7 years ago

Comment on attachment 8920746 [details] [diff] [review]
Like this

r=me

Attachment #8920746 - Flags: review?(bzbarsky) → review+

Assignee

Comment 21

•

7 years ago

Comment on attachment 8920746 [details] [diff] [review]
Like this

Let me know if I'm supposed to request approval for each affected branch too.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Probably not terribly easy. It's not easy to see a security issue straight from the patch, and the conditions to make the security issue happen are somewhat tricky.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No

Which older supported branches are affected by this flaw?
All

If not all supported branches, which bug introduced the flaw?
N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

They'll be fairly easy I think. This code hasn't changed that much, and I expect the patch to apply cleanly, or close to it.

How likely is this patch to cause regressions; how much testing does it need?
Not much, I think. This is just fixing an edge case where multiple flushes of the same shell aren't idempotent. We could add some automated tests post-landing, I think.

Attachment #8920746 - Flags: sec-approval?

Attachment #8920746 - Flags: approval-mozilla-esr52?

Comment 22

•

7 years ago

You should ask for Beta approval as well for 57.

I'm going to ping the Release Management team given how close to shipping we are before I give sec-approval.

Flags: needinfo?(rkothari)

Flags: needinfo?(lhenry)

Liz Henry (:lizzard) (relman/hg->git project)

Assignee

Comment 23

•

7 years ago

Comment on attachment 8920746 [details] [diff] [review]
Like this

Approval Request Comment
[Feature/Bug causing the regression]: bug 881832.
[User impact if declined]: Security issues.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not very risky
[Why is the change risky/not risky?]: "Just" does some work upfront to avoid invalidating data we store mid computation, but doesn't change any surrounding code other than that, so I think the behavior changes should mostly be limited to cases like the test-case.
[String changes made/needed]: none

Attachment #8920746 - Flags: approval-mozilla-release?

Attachment #8920746 - Flags: approval-mozilla-beta?

Comment 24

•

7 years ago

Though we already wontfixed this issue for 56, we are prepping a dot release for 56.  I'd like to leave it wontfixed since we only have a couple of weeks left in the release cycle.  Dan, what do you think?

Flags: needinfo?(lhenry) → needinfo?(dveditz)

Comment 25

•

7 years ago

We shouldn't shoehorn this into a 56.0.x. We've fixed a whole bunch of bugs since shipping 56 and there's nothing particularly special or more-easily-reverse-engineered about this one. Let it have the beta-baking we have left and release in a couple weeks with all the others.

Flags: needinfo?(dveditz)

Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)

Updated

•

7 years ago

tracking-firefox-esr52: ? → 57+

Comment 26

•

7 years ago

Hi Al, I believe we still have time to land sec-crits/highs in Beta57 as RC week begins Nov 6th. Please go ahead and approve for both beta and esr52 if you'd like.

Flags: needinfo?(rkothari)

Comment 27

•

7 years ago

Comment on attachment 8920746 [details] [diff] [review]
Like this

sec-approval, beta, and ESR52 approval given.

Attachment #8920746 - Flags: sec-approval?

Attachment #8920746 - Flags: sec-approval+

Attachment #8920746 - Flags: approval-mozilla-esr52?

Attachment #8920746 - Flags: approval-mozilla-esr52+

Attachment #8920746 - Flags: approval-mozilla-beta?

Attachment #8920746 - Flags: approval-mozilla-beta+

Comment 28

•

7 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c73db1c3b6e78a5fad216ca7cc7e4ce312c30a98

Comment 29

•

7 years ago

Backed out for a variety of test failures :(
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc9db2a3a28f6f441518a0a7d59e0dcea68c5e99

https://treeherder.mozilla.org/logviewer.html#?job_id=139313960&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=139313844&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=139313855&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=139313853&repo=mozilla-inbound

Assignee: matt.woodrow → emilio

Flags: needinfo?(emilio)

Assignee

Comment 30

•

7 years ago

Fun stuff, so those are flushes that should be idempotent, but clearly aren't... sigh

Boris, it's 1AM for me, but I'm moderately sure that removing the presShellForContent != mPresShell check is going to "fix" the tests... if that looks sane to you and you still have cycles left today, feel free to land it with that check removed.

Otherwise, I can look into it tomorrow.

Flags: needinfo?(bzbarsky)

Assignee

Comment 31

•

7 years ago

The other thing we can also do is land a MOZ_RELEASE_ASSERT(!presShell->IsDestroying()) to all branches, then fix it with more calm, of course... The out of date frames problem is not a sec issue I think, due to the frame poisoning stuff.

Assignee

Comment 32

•

7 years ago

So given that patch was on inbound, I did a couple try runs:

Patch as written:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e53552166d7a71ee9e42d5bfd479a03d7cf3a87

Second patch proposed (just for the sake of seeing what happened, really):

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=6522ffb16b4ccde3037aefeafb703a25f3e3b94d

With the fixup proposed in comment 30:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=57691a4b9b8bc000d4373bac4fa3d50b93b82e3c

Hiro, Birtles, looks like something is borked with animations in stylo, and this double flush in the same presShell in GetComputedStyle was wallpapering it. Mind taking a look?

Meanwhile, I'm not sure what's best... Seems like what's preventing me from landing the patch are stylo bugs, so the patch as written should work for ESR. I can figure out the failure in the third patch and see what's going on and land it, or land the release assertion...

Flags: needinfo?(hikezoe)

Flags: needinfo?(bbirtles)

Assignee

Comment 33

•

7 years ago

Ok, false alarm, I think... I mean, this is not a "flush is not idempotent" bug. This is an "our logic to avoid flushing is busted".

In particular, in that third patch set, the test failing is the test for bug 1363805. When I fix it, like:

  // In the FlushTarget::ParentOnly case we've proved that we don't need to do
  // any restyling, so don't bother.
  nsCOMPtr<nsIPresShell> presShellForContent = GetPresShellForContent(mContent);
  if (presShellForContent && target != FlushTarget::ParentOnly) {
    presShellForContent->FlushPendingNotifications(FlushType::Style);
  } else {
    MOZ_ASSERT(presShellForContent == mPresShell);
  }

Then that test starts passing, as expected, but all the same test failures come back again.

So the logic that detects those restyles for animations is busted, not the style flush in general, which is good news, because that patch is 58-only.

Assignee

Comment 34

•

7 years ago

So that means that:

 (1) We should still land the patch that got reviewed in branches, I think.
 (2) We should fix the ParentOnly logic.

Assignee

Comment 35

•

7 years ago

I don't understand why EffectCompositor::HasPendingStyleUpdatesFor only needs to check the element itself, and not any of its ancestors. That looks very wrong.

Comment 36

•

7 years ago

(In reply to Emilio Cobos Álvarez [:emilio] from comment #35)
> I don't understand why EffectCompositor::HasPendingStyleUpdatesFor only
> needs to check the element itself, and not any of its ancestors. That looks
> very wrong.

Oh right.  Actually it does not match what the comment says.  

  // If any ancestor has pending animation, flush it.
  nsPresContext* context = shell->GetPresContext();                             
  if (context->EffectCompositor()->HasPendingStyleUpdatesFor(aElement)) {       
    return true;                                                                
  }

Comment 37

•

7 years ago

I did look the failure on test_csstransition-transitionproperty.html, it works if the following change is applied.

diff --git a/dom/animation/test/css-transitions/file_csstransition-transitionproperty.html b/dom/animation/test/css-transitions/file_csstransition-transitionproperty.html
--- a/dom/animation/test/css-transitions/file_csstransition-transitionproperty.html
+++ b/dom/animation/test/css-transitions/file_csstransition-transitionproperty.html
@@ -10,7 +10,7 @@ test(function(t) {
 
   // Add a transition
   div.style.left = '0px';
-  getComputedStyle(div).transitionProperty;
+  getComputedStyle(div).left;
   div.style.transition = 'all 100s';
   div.style.left = '100px';

I don't know much about the optimization what was done in bug 1363805,  bug as for this test case, I'd say we should fix the test case itself.

Assignee

Comment 38

•

7 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #37)
>    // Add a transition
>    div.style.left = '0px';
> -  getComputedStyle(div).transitionProperty;
> +  getComputedStyle(div).left;
>    div.style.transition = 'all 100s';
>    div.style.left = '100px';
> 
> I don't know much about the optimization what was done in bug 1363805,  bug
> as for this test case, I'd say we should fix the test case itself.

The test-case should work without that change. That change forces a flush, which "fixes" it, but it shouldn't be needed. The test-case is correct as far as I can tell (we should check other browsers though)...

I think this is the real problem:

After looking a bit more into it, it's not only what I mentioned re. EffectCompositor not looking for ancestors, but the fact that for animations to get triggered, we rely on the traversal machinery, and thus the flush.

So bug 1363805 is completely busted :(

Basically, what the tests that are failing are doing is creating a <div style="animation: foo 1s"></div>, append to the doc, and expect the animations to apply.

The element has no Servo data and hasn't been traversed, but we still skip the flush because it's right that no element in the document needs an style update... So we never trigger the animation before resolving the style, which means that we return the unanimated style...

Assignee

Comment 39

•

7 years ago

I've got a fix. Gah, this is annoying.

Comment 40

•

7 years ago

(In reply to Emilio Cobos Álvarez [:emilio] from comment #38)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #37)
> >    // Add a transition
> >    div.style.left = '0px';
> > -  getComputedStyle(div).transitionProperty;
> > +  getComputedStyle(div).left;
> >    div.style.transition = 'all 100s';
> >    div.style.left = '100px';
> > 
> > I don't know much about the optimization what was done in bug 1363805,  bug
> > as for this test case, I'd say we should fix the test case itself.
> 
> The test-case should work without that change. That change forces a flush,
> which "fixes" it, but it shouldn't be needed. The test-case is correct as
> far as I can tell (we should check other browsers though)...

Yeah, it seems you are right. chrome does trigger the transition.

> I think this is the real problem:
> 
> After looking a bit more into it, it's not only what I mentioned re.
> EffectCompositor not looking for ancestors, but the fact that for animations
> to get triggered, we rely on the traversal machinery, and thus the flush.
> 
> So bug 1363805 is completely busted :(
> 
> Basically, what the tests that are failing are doing is creating a <div
> style="animation: foo 1s"></div>, append to the doc, and expect the
> animations to apply.
> 
> The element has no Servo data and hasn't been traversed, but we still skip
> the flush because it's right that no element in the document needs an style
> update... So we never trigger the animation before resolving the style,
> which means that we return the unanimated style...

I think this issue is not related to animations, for example;

element.style.left = '0px';
getComputedStyle(element).transitionProperty;

This code does not trigger any traversals as far as I can see with RUST_LOG=debug.

Instead getComputedStyle(element).left triggers traversals.

Oh, good to hear that Emilo fixed this. :)

Flags: needinfo?(hikezoe)

Assignee

Comment 41

•

7 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #40)
> I think this issue is not related to animations, for example;
> 
> element.style.left = '0px';
> getComputedStyle(element).transitionProperty;
> 
> This code does not trigger any traversals as far as I can see with
> RUST_LOG=debug.
> 
> Instead getComputedStyle(element).left triggers traversals.

Right, the difference is that `.left` is a layout-dependent property, and triggers an unconditional flush. Not triggering traversals is exactly the optimization that bug 1363805 implemented, it just didn't account for the side-effect of triggering animations.

Assignee

Comment 42

•

7 years ago

Attached patch Fix detection of animations to avoid flush. — Details — Splinter Review

This fixes it. See the commit message for details. Hiro, Cam is away, do you think you can review it?

Also, note that this patch is trunk-only, in the rest the previous patch should apply fine and work fine.

Flags: needinfo?(emilio)

Attachment #8921838 - Flags: review?(hikezoe)

Assignee

Comment 43

•

7 years ago

(I guess now that I figured that out I'm not scared about non-idempotent flushes anymore...)

Flags: needinfo?(bzbarsky)

Flags: needinfo?(bbirtles)

Comment 44

•

7 years ago

Comment on attachment 8921838 [details] [diff] [review]
Fix detection of animations to avoid flush.

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

Looks reasonable to me.  Thanks!

::: layout/style/nsComputedDOMStyle.cpp
@@ +111,4 @@
>    MOZ_ASSERT(aContent);
>    nsIContent* node = aContent;
>    while (node) {
> +    if (node->IsElement()) {

We can also check MayHaveAnimations() here.

Attachment #8921838 - Flags: review?(hikezoe) → review+

Comment 45

•

7 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #44)
> Comment on attachment 8921838 [details] [diff] [review]
> Fix detection of animations to avoid flush.
> 
> Review of attachment 8921838 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks reasonable to me.  Thanks!
> 
> ::: layout/style/nsComputedDOMStyle.cpp
> @@ +111,4 @@
> >    MOZ_ASSERT(aContent);
> >    nsIContent* node = aContent;
> >    while (node) {
> > +    if (node->IsElement()) {
> 
> We can also check MayHaveAnimations() here.

Oh, never mind. We do it in EffectSet::GetEffectSet().

Assignee

Comment 46

•

7 years ago

For trunk:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=df307dd71a856300ba928fab81a2f1143fe1cb74

Seems like this needs a fix for OMTA animations...

Assignee

Comment 47

•

7 years ago

Attached patch Fix for OMTA animations. r=birtles — Details — Splinter Review

Birtles reviewed this with me as I wrote this. This should make the tests green. Let me verify it and I'll start landing this.

Attachment #8921884 - Flags: review+

Assignee

Comment 48

•

7 years ago

Ryan landed this morning the first patch in beta and it's green as expected.

The patches here are also green, so will land them as soon as https://github.com/servo/servo/pull/19016 merges into autoland.

Assignee

Comment 49

•

7 years ago

uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/39a3a1dd8c6b87d09b9d4f8959eeb8e905d529a2

status-firefox57: affected → fixed

Comment 50

•

7 years ago

This'll need a rebased patch for ESR52.

Flags: needinfo?(emilio)

Assignee

Comment 51

•

7 years ago

Attached patch ESR patch. (obsolete) — Details — Splinter Review

Flags: needinfo?(emilio)

Assignee

Comment 52

•

7 years ago

Attached patch ESR patch, but building. — Details — Splinter Review

Attachment #8921950 - Attachment is obsolete: true

Assignee

Comment 53

•

7 years ago

This had servo bits, so I had to push them to autoland.

Servo changes: https://hg.mozilla.org/integration/autoland/rev/c7db33cbe5de

Gecko changes:

  https://hg.mozilla.org/integration/autoland/rev/0b1f15ab849b632550502347d8f306326c73b55b
  https://hg.mozilla.org/integration/autoland/rev/ece6798a15ba1a94c497166142dc84e74a157471

Comment 54

•

7 years ago

https://hg.mozilla.org/mozilla-central/rev/0b1f15ab849b
https://hg.mozilla.org/mozilla-central/rev/ece6798a15ba

Status: NEW → RESOLVED

Closed: 7 years ago

status-firefox58: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla58

Comment 55

•

7 years ago

uplift

https://hg.mozilla.org/releases/mozilla-esr52/rev/5ddd5d2aa769

status-firefox-esr52: affected → fixed

Updated

•

7 years ago

Group: layout-core-security → core-security-release

Updated

•

7 years ago

Depends on: 1412252

Liz Henry (:lizzard) (relman/hg->git project)

Updated

•

7 years ago

Flags: sec-bounty? → sec-bounty+

Comment 57

•

7 years ago

Comment on attachment 8920746 [details] [diff] [review]
Like this

Letting this ride to release with 57 based on comment 25.

Attachment #8920746 - Flags: approval-mozilla-release? → approval-mozilla-release-

Updated

•

7 years ago

Whiteboard: [adv-main57+][adv-esr52.5+]

Matt Wobensmith [:mwobensmith][:matt:]

Updated

•

7 years ago

Alias: CVE-2017-7828

Comment 58

•

7 years ago

Confirmed issue on Fx56.0.2.
Verified fixed on Fx57.0b14 and Fx52.5.0esr.

Status: RESOLVED → VERIFIED

Flags: qe-verify+

Whiteboard: [adv-main57+][adv-esr52.5+] → [adv-main57+][adv-esr52.5+][post-critsmash-triage]

Emil Ghitta, QA [:emilghitta]

Comment 59

•

6 years ago

I have managed to reproduce the issue mentioned in comment 0 using Firefox 58.0a1 (BuildId:20171008060810).

This issue is no longer reproducible using Firefox 57.0.3 (BuildId:20171217111436), 58.0b12 (BuildId:20171215145727), 59.0a1 (BuildId:20171217094207) and 52.5.3 esr (BuildId:20171217192058) ASAN builds on Ubuntu 16.04 64bit.

status-firefox57: fixed → verified

status-firefox58: fixed → verified

status-firefox59: --- → verified

status-firefox-esr52: fixed → verified

Flags: qe-verify+