Crash in mozilla::a11y::Accessible::SetARIAHidden

RESOLVED FIXED in Firefox -esr52

Status

()

defect
P2
critical
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: philipp, Assigned: surkov)

Tracking

(Blocks 2 bugs, 4 keywords)

51 Branch
mozilla55
All
Windows
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr45 wontfix, firefox51 wontfix, firefox52+ wontfix, firefox-esr5255+ fixed, firefox53+ wontfix, firefox54+ wontfix, firefox55 verified)

Details

(Whiteboard: [adv-main55+][adv-esr52.3+][post-critsmash-triage], crash signature)

Attachments

(3 attachments, 2 obsolete attachments)

This bug was filed from the Socorro interface and is 
report bp-23d9a615-5bf1-4994-b600-66af02161130.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::a11y::Accessible::SetARIAHidden(bool) 	accessible/generic/Accessible.cpp:2536
1 	xul.dll 	mozilla::a11y::Accessible::SetARIAHidden(bool) 	accessible/generic/Accessible.cpp:2538
2 	xul.dll 	mozilla::a11y::Accessible::SetARIAHidden(bool) 	accessible/generic/Accessible.cpp:2538
3 	xul.dll 	mozilla::a11y::Accessible::SetARIAHidden(bool) 	accessible/generic/Accessible.cpp:2538
4 	xul.dll 	mozilla::a11y::Accessible::SetARIAHidden(bool) 	accessible/generic/Accessible.cpp:2538
5 	xul.dll 	mozilla::a11y::Accessible::SetARIAHidden(bool) 	accessible/generic/Accessible.cpp:2538
6 	xul.dll 	mozilla::a11y::Accessible::SetARIAHidden(bool) 	accessible/generic/Accessible.cpp:2538
7 	xul.dll 	mozilla::a11y::Accessible::SetARIAHidden(bool) 	accessible/generic/Accessible.cpp:2538
8 	xul.dll 	mozilla::a11y::Accessible::SetARIAHidden(bool) 	accessible/generic/Accessible.cpp:2538
9 	xul.dll 	mozilla::a11y::Accessible::SetARIAHidden(bool) 	accessible/generic/Accessible.cpp:2538
10 	xul.dll 	mozilla::a11y::Accessible::SetARIAHidden(bool) 	accessible/generic/Accessible.cpp:2538
11 	xul.dll 	mozilla::a11y::DocAccessible::ARIAAttributeChanged(mozilla::a11y::Accessible*, nsIAtom*) 	accessible/generic/DocAccessible.cpp:984
12 	xul.dll 	mozilla::a11y::DocAccessible::AttributeChangedImpl(mozilla::a11y::Accessible*, int, nsIAtom*) 	accessible/generic/DocAccessible.cpp:837
13 	xul.dll 	mozilla::a11y::DocAccessible::AttributeChanged(nsIDocument*, mozilla::dom::Element*, int, nsIAtom*, int, nsAttrValue const*) 	accessible/generic/DocAccessible.cpp:774
14 	xul.dll 	mozilla::dom::Element::SetAttrAndNotify(int, nsIAtom*, nsIAtom*, nsAttrValue const&, nsAttrValue&, unsigned char, bool, bool, bool) 	dom/base/Element.cpp:2520

crashes with this signature are rising in frequency since firefox 51 and subsequent builds. they occur on all versions of windows with 32bit and 64bit and are low volume, so there are no correlations generated for the signature (i don't see something obvious as e10s sticking out either).

the only recorded user comment for it so far sais they were researching something on ancestry.co.uk
Alex, can you take a look?
Flags: needinfo?(surkov.alexander)
it looks there's a tree update problem, due to some reason mChildren contains a dead accessible

Can we have a regression bug and/or steps to reproduce (I'll trying to play with ancestry.co.uk for sure)?
Flags: needinfo?(surkov.alexander)
Is there a speculative fix we can do here?
Flags: needinfo?(surkov.alexander)
Hi Alex,
From the report, it seems to me that the bug starts to show up from 51.0b6. Do you think if any changes between beta 5 and beta 6 caused this issue?
(In reply to David Bolter [:davidb] from comment #3)
> Is there a speculative fix we can do here?

There's no speculative fix I can think of, and no ideas so far. If that is a broken tree, then there's only way to fix the bug, is to find a cause of tree brokenness. It's hard to overestimate the importance of str on this way.

(In reply to Gerry Chang [:gchang] from comment #4)
> Hi Alex,
> From the report, it seems to me that the bug starts to show up from 51.0b6.
> Do you think if any changes between beta 5 and beta 6 caused this issue?

is there any handy link at hands I can run through and check if a bell rings?
Flags: needinfo?(surkov.alexander)
Hi Alex, 
https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=9afe68360fa8&tochange=2dec3c6c7c90
This is the changes between beta 5 and beta 6. I'm not sure if this helps.
Flags: needinfo?(surkov.alexander)
Priority: -- → P2
(In reply to Gerry Chang [:gchang] (OOO 26 Dec - 10 Jan) from comment #6)
> Hi Alex, 
> https://hg.mozilla.org/releases/mozilla-beta/
> pushloghtml?fromchange=9afe68360fa8&tochange=2dec3c6c7c90
> This is the changes between beta 5 and beta 6. I'm not sure if this helps.

There's a couple of DOM and layout changes there, which might affect but shouldn't, and nothing directly affiliating with a11y.

Is there a talent at Mozilla who could create a test case or hunt down the steps to reproduce the crash?
Flags: needinfo?(surkov.alexander)
This is a UAF

Also, signatures on both 53 and 54; set to affected and 51/52 as well since we now know it's a UAF.
Alex can you take another look here?
Flags: needinfo?(dbolter) → needinfo?(surkov.alexander)
I did see one UAF crash on ESR-45.3 in December so this might go back at least that far. There's no question this exploded with 51 and wasn't happening much on Firefox 50 (2 UAF crashes in January, 1 in December). Two different bugs? Or a change elsewhere that makes the same bug more likely to happen?
Group: core-security → dom-core-security
Keywords: sec-high
(In reply to Daniel Veditz [:dveditz] from comment #11)
> I did see one UAF crash on ESR-45.3 in December so this might go back at
> least that far. There's no question this exploded with 51 and wasn't
> happening much on Firefox 50 (2 UAF crashes in January, 1 in December). Two
> different bugs? Or a change elsewhere that makes the same bug more likely to
> happen?

I would suspect a change elsewhere that reveals the bug more often. It looks like an accessible tree update problem, which depends on layout notifications, which means the changes in layout may affect on the bug ratio. On the other hand, it also may be related with the web services changes, aria-hidden is not a thing used everywhere on the web.

(In reply to David Bolter [:davidb] from comment #10)
> Alex can you take another look here?

We don't have many good options to choose from. Here's how we can approach to the problem.

* Steps to reproduce - if anyone on the Mozilla's earth is capable to reproduce it reliably, that'd be a 90% of the problem fix.

* Inspect the code - I run through the code once again, and couldn't spot any possible issue. I realize that my eyes can be fairly soaped in this area, so here are the steps, if someone wants to give it a try.

I think the problem is Accessible::mChildren contains a reference on a dead accessible. mParent/mChildren changing looks synchronized through the code, which happens in InsertChildAt/RemoveChild methods. If accessible gets shutdown for any reason (Accessible::Shutdown), then it also get removed from its parent. If anyone is able to spot a problem here, then it can be also a fix.

* We may kill this specific crash, not solving a root problem. There are two options on this way:

** Cut off aria-hidden trees: no tree - nowhere to crash. There's still ongoing discussion whether this is a right thing to do.

** Keep strong references in mChildren. That means the memory increase. We never bothered to make memory measurements for a11y, so no numbers here, but I suppose this'd be a significant increase. On a good side, no crashes, just broken tree and possibly broken user experience.

* Also it may be worth to try to replace mChildren array on a linked list. I wanted to get it done anyways to improve performance of tree updates. This is not a solution by itself of course, but it may solve this particular problem, because all related code will be reworked on this way.
Flags: needinfo?(surkov.alexander)
(In reply to alexander :surkov from comment #12)
> (In reply to Daniel Veditz [:dveditz] from comment #11)

> ** Cut off aria-hidden trees: no tree - nowhere to crash. There's still
> ongoing discussion whether this is a right thing to do.

This would make us more interoperable. Do you have opinion on what kind of new stability risk the change could introduce?

Do we need to walk the whole subtree setting aria-hidden? AT often walk the parent chain anyway, so presumably they could also check aria-hidden...

This bug might be bad enough to warrant changing this behavior.
Flags: needinfo?(surkov.alexander)
Cut-the-tree indeed will get rid of this crash signature, and will reduce crash ratio overall. The problem though is about broken tree, which means you should have similar crashes in other places.

Google concerned about cut-the-tree approach, trying to figure out if there are alternatives. So I'd be reluctant to jump into trying different approaches, until the investigation/discussion is done.

I recall that aria-hidden state propagation though the tree was agreed with AT, however info about this seems to be scattered over many places. Here's an example of a bit, https://bugzilla.mozilla.org/show_bug.cgi?id=780888#c17.

Anyway reducing the crash ratio may be a poor justification for changing the behavior overall. It feels like solving the problem from a wrong side.

Can we get Mozilla QA's running through the crash-stats comments chasing steps for the crash? If we had steps, then we are able to solve the bigger problem.
Flags: needinfo?(surkov.alexander) → needinfo?(dbolter)
Adding Florin to the cc list to address the request for testing in Comment 14.
many of the recent user comments talk about emptying their junk folder in outlook/hotmail when the crash happened: http://bit.ly/2kUbIOC
Andrei, could you add this bug to the list of issues that our team investigates? Maybe we can get some time for someone to have a look at it, and try to reproduce this crash.
Flags: needinfo?(andrei.vaida)
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #17)
> Andrei, could you add this bug to the list of issues that our team
> investigates? Maybe we can get some time for someone to have a look at it,
> and try to reproduce this crash.

Added to our todo list. Keeping the ni? in place until this is done.
Posted patch patchSplinter Review
This might help.

Not sure how we get into this, but if we do, then it's possible scenario. Ugly but low risk, and if helps, then can be backported.

I'm getting confident that linked lists is a right fix of the bigger problem.
Attachment #8835531 - Flags: review?(eitan)
Flags: needinfo?(dbolter)
Comment on attachment 8835531 [details] [diff] [review]
patch

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

Looks good.

re:linked list. Wouldn't you have the same problem? The mIndexInParent would need to be kept in sync.
Attachment #8835531 - Flags: review?(eitan) → review+
(In reply to Eitan Isaacson [:eeejay] from comment #20)

> Looks good.

thanks

> re:linked list. Wouldn't you have the same problem? The mIndexInParent would
> need to be kept in sync.

Technically not, because you don't need index to update the tree, since each child knows about its siblings. Also, I was thinking to get rid mIndexInParent at all, since we don't use it much internally, but keeping it updated goes at cost. In case if ATs relies on it a lot, I think we could try smart things, like keeping the last used index cached.
Comment on attachment 8835531 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch? not easy, I wish I would know myself how to do this

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? no, no tests, I was thinking about 'Bug - attempt to fix a wrong index issue on accessible child removal'

Which older supported branches are affected by this flaw? 45 was reported, but spiked in 51

If not all supported branches, which bug introduced the flaw? unknown

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

How likely is this patch to cause regressions; how much testing does it need? it's fairly unrisky since it takes care about edge cases.
Attachment #8835531 - Flags: sec-approval?
sec-approval+ for trunk. We'll want branch patches nominated as well after it goes into trunk.
Attachment #8835531 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/69404501d4d8fc26cc4317ccaec8cf997e7b599a
Bug 1321384 - attempt to fix a wrong index issue on accessible child removal, r=eeejay
https://hg.mozilla.org/mozilla-central/rev/69404501d4d8
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Please request Aurora/Beta approval on this when you get a chance.
Assignee: nobody → surkov.alexander
Flags: needinfo?(surkov.alexander)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #26)
> Please request Aurora/Beta approval on this when you get a chance.

I'm not still 100% sure that the patch fixes the crash. The patch however should be harmful enough to get backport it, if it helps to prove or disprove the patch fixes the problem.
Flags: needinfo?(surkov.alexander)
Comment on attachment 8835531 [details] [diff] [review]
patch

Approval Request Comment
[Feature/Bug causing the regression]:unknown
[User impact if declined]:crashes, security risks
[Is this code covered by automated tests?]:no
[Has the fix been verified in Nightly?]:no
[Needs manual test from QE? If yes, steps to reproduce]: no steps to reproduce
[List of other uplifts needed for the feature/fix]:no
[Is the change risky?]:low risk
[Why is the change risky/not risky?]:handling edge cases, which presumably may happen at unknown circumstances, the patch itself is rather simple
[String changes made/needed]:no
Attachment #8835531 - Flags: approval-mozilla-aurora?
Comment on attachment 8835531 [details] [diff] [review]
patch

Fix a crash & sec-high. Aurora53+.
Attachment #8835531 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8835531 [details] [diff] [review]
patch

See comment 28.
Attachment #8835531 - Flags: approval-mozilla-beta?
Group: dom-core-security → core-security-release
Comment on attachment 8835531 [details] [diff] [review]
patch

avoid crash/uaf, beta52+

Should be in 52.0b7.
Attachment #8835531 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-esr52/rev/5701c0b8b4b8

Did we want to get this on esr45 still?
Flags: needinfo?(surkov.alexander)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #34)
> https://hg.mozilla.org/releases/mozilla-esr52/rev/5701c0b8b4b8
> 
> Did we want to get this on esr45 still?

It seems there's no 45 crashes in [1], however they occurred in the past per comment 11.

I'm not sure yet if the patch helps or there's an evidence it decreases the crash ratio. [1] has a 53.0a2 20170216004023 build, however the aurora patch was landed feb 15 per comment 31.

[1] https://crash-stats.mozilla.com/signature/?product=Firefox&signature=mozilla%3A%3Aa11y%3A%3AAccessible%3A%3ASetARIAHidden&date=%3E%3D2017-02-10T15%3A13%3A00.000Z&date=%3C2017-02-17T15%3A13%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_sort=-version&_sort=-build_id&_sort=-date&page=1
Flags: needinfo?(surkov.alexander)
Flags: needinfo?(andrei.vaida)
I tried to reproduce this crash using Firefox 51.0.1 on Windows 10 x86 and x64 with main focus on Outlook, Yahoo and Gmail email clients (based on the comments from the users that ran into this issue) but with no success. 
Please let me know if there is anything I can help with here.
there are still crashes with this signature in 52.0b7 - should we reopen the bug?
http://bit.ly/2m4mbFD
Flags: needinfo?(surkov.alexander)
Reopening preemptively; no apparent change in b7.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Randell Jesup [:jesup] from comment #38)
> Reopening preemptively; no apparent change in b7.

Yep.

I'm curious though if there is a correlation between the landed patch and crashes ratio. I see the last reported crash is dated by Feb 17 on 53.0a2 and Feb 16 on 52.0b7.

I can see another possible path for bad indexes, which is worth to investigate. I'll file a patch shortly.
Flags: needinfo?(surkov.alexander)
it'd be interesting to see if there's a problem here as well
Attachment #8839586 - Flags: review?(eitan)
Attachment #8839586 - Flags: review?(eitan) → review+
Comment on attachment 8839586 [details] [diff] [review]
diagnostics for Move

[Security approval request comment]
How easily could an exploit be constructed based on the patch? no working ideas

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

Which older supported branches are affected by this flaw? all

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

How likely is this patch to cause regressions; how much testing does it need?
Attachment #8839586 - Flags: sec-approval?
Comment on attachment 8839586 [details] [diff] [review]
diagnostics for Move

sec-approval+ for trunk.
Does this need to go to branches? The last beta is tomorrow.
Attachment #8839586 - Flags: sec-approval? → sec-approval+
(In reply to Al Billings [:abillings] from comment #42)
> Comment on attachment 8839586 [details] [diff] [review]
> diagnostics for Move
> 
> sec-approval+ for trunk.
> Does this need to go to branches? The last beta is tomorrow.

I didn't see crashes on nightly, so I need to backport it at least to aurora, to grab more data
Keywords: leave-open
Crashes on 51 and newer.  Rate is low on 52 and 53, due to lower usage probably, so 54 might not see a crash (yet).  I's assume it's on all versions from 51 on.
Comment on attachment 8839586 [details] [diff] [review]
diagnostics for Move

Approval Request Comment
[Feature/Bug causing the regression]:unknown
[User impact if declined]:no impact as the patch is diagnostic one
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:no
[Is the change risky?]:no
[Why is the change risky/not risky?]:assertions only
[String changes made/needed]:no
Attachment #8839586 - Flags: approval-mozilla-aurora?
Comment on attachment 8839586 [details] [diff] [review]
diagnostics for Move

Diagnostic patch for crash, let's uplift to aurora.
Attachment #8839586 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
It seems MoveChild doesn't cause this crash, I don't see any MoveChild crashes, but I do see new SetARIAHideen on 53a2 with diagnostic asserts on, for example, https://crash-stats.mozilla.com/report/index/c2c8cf1b-f0f8-4ef6-8445-652d92170306
Posted patch RemoveChild diagnostics (obsolete) — Splinter Review
Attachment #8844898 - Flags: review?(yzenevich)
Comment on attachment 8844898 [details] [diff] [review]
RemoveChild diagnostics

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

Looks good as long as we don't intend to bail on some of those conditions.
Attachment #8844898 - Flags: review?(yzenevich) → review+
Posted patch RemoveChild diagnostics (obsolete) — Splinter Review
Attachment #8844898 - Attachment is obsolete: true
Attachment #8844905 - Flags: review?(yzenevich)
(In reply to alexander :surkov from comment #53)
> Created attachment 8844905 [details] [diff] [review]
> RemoveChild diagnostics

I've got that thing on my local build, I'll take a look at it.

Yura, I suspect you may experience same problem with ISimpleDOMNode issue debugging.
fix XUL tree item/cells shutdown to not attempt to remove the parent/child relations (since they are not used)
Attachment #8844905 - Attachment is obsolete: true
Attachment #8844905 - Flags: review?(yzenevich)
Attachment #8844999 - Flags: review?(yzenevich)
Comment on attachment 8844999 [details] [diff] [review]
RemoveChild diagnostics

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

looks good thanks
Attachment #8844999 - Flags: review?(yzenevich) → review+
Comment on attachment 8844999 [details] [diff] [review]
RemoveChild diagnostics

[Security approval request comment]
How easily could an exploit be constructed based on the patch? not easy

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

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? same patch

How likely is this patch to cause regressions; how much testing does it need? unlikely
Attachment #8844999 - Flags: sec-approval?
Comment on attachment 8844999 [details] [diff] [review]
RemoveChild diagnostics

sec-approval+
Attachment #8844999 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/dea9f3038c4d

Unsure where this leaves status flags, milestones, resolution.
Depends on: 1346518
Fwiw, this signature also shows up in EXCEPTION_STACK_OVERFLOW crashes:
bp-f4820ef9-35bd-44a4-ba38-c0ecf2170312
(In reply to Mats Palmgren (:mats) from comment #61)
> Fwiw, this signature also shows up in EXCEPTION_STACK_OVERFLOW crashes:
> bp-f4820ef9-35bd-44a4-ba38-c0ecf2170312

Ugh. Would this point to a cycle in the tree or?

(I feel like we're paying a heavy tax implementing aria-hidden this way)
(In reply to David Bolter [:davidb] from comment #62)
> (In reply to Mats Palmgren (:mats) from comment #61)
> > Fwiw, this signature also shows up in EXCEPTION_STACK_OVERFLOW crashes:
> > bp-f4820ef9-35bd-44a4-ba38-c0ecf2170312
> 
> Ugh. Would this point to a cycle in the tree or?

looks so

> (I feel like we're paying a heavy tax implementing aria-hidden this way)

Arguably, this is a tax for having wrong trees. We have other bugs caused by same issue. Even if we've got aria-hidden implemented other way, then it doens't guarantee that other stacks wont' be spiked by doing this.
Alexander, does this updated patch still need to land for aurora 54 and beta 53?
Flags: needinfo?(surkov.alexander)
Also, do we need to uplift anything from bug 1346518 or other bugs mentioned there as crash causes?
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #64)
> Alexander, does this updated patch still need to land for aurora 54 and beta
> 53?

not yet, we have debugging crashes on nightly, which should be addressed before doing this

(In reply to Randell Jesup [:jesup] from comment #65)
> Also, do we need to uplift anything from bug 1346518 or other bugs mentioned
> there as crash causes?

bug 1342433, which get rids a code path of getting to a broken tree (not the broken tree problem fix itself though) was backported to aurora (54), it might be a candidate for a beta. No much sense to backport assert patches from bug 1346518 yet, since they crash on nightly.

Note, bug 1349223 will get rid this crash signature, probably moving the crashes to other place like bug 1318645. Not sure, if it should be backported or not, since it is ARIA spec implementation change.
Flags: needinfo?(surkov.alexander)
ok, we'll want any beta updates this week or perhaps next, since we're at beta 6 currently
We're at beta 8 or 9 now... we need to fix this in 53 or wontfix it.
Flags: needinfo?(surkov.alexander)
Flags: needinfo?(dbolter)
Alex are there any low risk fixes or mitigations we should try to take now for 53?
Flags: needinfo?(dbolter)
(In reply to David Bolter [:davidb] from comment #69)
> Alex are there any low risk fixes or mitigations we should try to take now
> for 53?

I can't see the crashes on 54+, I recall though I saw some on aurora channel previously. Might be bug 1342433, which went to 54, is a mitigation of the problem. Let me know if I should go asking to backport it to 53 (beta).
Flags: needinfo?(surkov.alexander)
We may want to uplift this to beta, Alexander, what do you think? We do see some crashes on 53 and significantly more on 52 release so I expect this will be higher volume on 53 release.
Flags: needinfo?(surkov.alexander)
(In reply to alexander :surkov from comment #70)
> I can't see the crashes on 54+, I recall though I saw some on aurora channel
> previously. Might be bug 1342433, which went to 54, is a mitigation of the
> problem.
yes, it looks like there are no more of those crash reports recorded after 54.0a2 build 20170320004004
Are we going to take this on ESR52 and ESR45?
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Flags: needinfo?(jcristau)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #71)
> We may want to uplift this to beta, Alexander, what do you think? We do see
> some crashes on 53 and significantly more on 52 release so I expect this
> will be higher volume on 53 release.

(In reply to [:philipp] from comment #72)
> (In reply to alexander :surkov from comment #70)
> > I can't see the crashes on 54+, I recall though I saw some on aurora channel
> > previously. Might be bug 1342433, which went to 54, is a mitigation of the
> > problem.
> yes, it looks like there are no more of those crash reports recorded after
> 54.0a2 build 20170320004004

done, asked bug bug 1342433 for uplifting, hope it will help to this one
Flags: needinfo?(surkov.alexander)
We already landed the patch in bug 1342433 on m-c including the tests, so I don't think sec approval is needed there. Al is that correct?
Flags: needinfo?(lhenry) → needinfo?(abillings)
I've approved the patch in 1342433 for esr52, I'm a bit hesitant about esr45 since there don't seem to be many crashes there and there's some identified risk.
Flags: needinfo?(rkothari)
Flags: needinfo?(jcristau)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #75)
> We already landed the patch in bug 1342433 on m-c including the tests, so I
> don't think sec approval is needed there. Al is that correct?

Correct.
Flags: needinfo?(abillings)
Here, it looks like the patch from bug 1342433 caused the current top crash in beta 10 (1000 crashes in a day, very high volume).  I think we need to back it out.
We are still seeing some crashes with this signature on 53.0 release.
I don't think that's unexpected since bug 1342433 was backed out from 53/54. Not seeing any reports from 55, though, which is encouraging at least. Can we call this bug FIXED?

Also, we're out of planned releases for ESR45, so marking that as wontfix at this point.
Target Milestone: mozilla54 → mozilla55
Still seeing no 55 crashes.  Still crashes in 54b4; waiting on a patch for bug 1342433 for 54 to land
Still no 55 crashes, so I'll tentatively call this fixed.  But we're running out of time for 54, so wontfix there.
Status: REOPENED → RESOLVED
Closed: 3 years ago2 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Alexander, why is this re-opened when we're getting no crashes in 55? Can't we track 54 issues in bug 1363027 unless we're getting crashes in 55 now?
Flags: needinfo?(surkov.alexander)
Agreed, we should close, fixed in 55. Alex can you prepare a patch for ESR 52 and request approval?
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Flags: needinfo?(dbolter)
Resolution: --- → FIXED
(In reply to Al Billings [:abillings] from comment #85)
> Alexander, why is this re-opened when we're getting no crashes in 55? Can't
> we track 54 issues in bug 1363027 unless we're getting crashes in 55 now?

it seems bug 1342433 (55 only) and bug 1363027 (backported up to 52) is a fix mixture for this bug. I'm good to keep it closed since it appears fixed on trunk, not sure what is a right place to keep tracking of <=54 crashes.

(In reply to David Bolter [:davidb] from comment #86)
> Agreed, we should close, fixed in 55. Alex can you prepare a patch for ESR
> 52 and request approval?

which patch for esr?
Flags: needinfo?(surkov.alexander)
Ryan not sure if you were aware of the sec aspect here... (re: bug 1342433 comment 32)? I think we should try to get sec bug fixes into ESR...
Flags: needinfo?(surkov.alexander)
Flags: needinfo?(ryanvm)
Flags: needinfo?(dbolter)
(In reply to David Bolter [:davidb] from comment #89)
> Ryan not sure if you were aware of the sec aspect here... (re: bug 1342433
> comment 32)? I think we should try to get sec bug fixes into ESR...

that one was backed out from all branches but trunk because it was mystically related with spiking this crash. Honestly I don't have any actionable ideas.
Flags: needinfo?(surkov.alexander)
These various a11y crash/sec bugs have left me a bit lost as to what needs to go where at this point, TBH. I would just prefer that those who understand this code are able to make those judgements at this point so we can clear up the status one way or another.
Flags: needinfo?(ryanvm)
Any updates on an ESR patch for this? We're running out of runway for the release.
I guess the question boils down to whether we want to uplift bug 1342433 or not (and whether we think it's likely to cause other issues if we do). Alexander, is that something you can speak to?
Flags: needinfo?(surkov.alexander)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #93)
> I guess the question boils down to whether we want to uplift bug 1342433 or
> not (and whether we think it's likely to cause other issues if we do).
> Alexander, is that something you can speak to?

That bug was known for spiking the crashes up, for mystical reasons with me, what worries me. We backported a lot of goodies, including bug 1363027, so it should be ok to backport it as well. Is this crash bites hard esr52 users?
Flags: needinfo?(surkov.alexander)
537 crashes on ESR 52.2.1 in the last week according to crash-stats.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #95)
> 537 crashes on ESR 52.2.1 in the last week according to crash-stats.

seems high enough, move the discussion into bug 1342433 then?
Whiteboard: [adv-main55+]
We've uplifted bug 1363027 to ESR52 as well now. I think we can call this bug as finished as it'll ever be.
Whiteboard: [adv-main55+] → [adv-main55+][adv-esr52.3+]
Marking verified for Fx 55 as per comment 82 and beyond.
Flags: qe-verify-
Whiteboard: [adv-main55+][adv-esr52.3+] → [adv-main55+][adv-esr52.3+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.