Closed
Bug 1321384
Opened 8 years ago
Closed 7 years ago
Crash in mozilla::a11y::Accessible::SetARIAHidden
Categories
(Core :: Disability Access APIs, defect, P2)
Tracking
()
People
(Reporter: philipp, Assigned: surkov)
References
(Blocks 2 open bugs)
Details
(4 keywords, Whiteboard: [adv-main55+][adv-esr52.3+][post-critsmash-triage])
Crash Data
Attachments
(3 files, 2 obsolete files)
(deleted),
patch
|
eeejay
:
review+
gchang
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
eeejay
:
review+
lizzard
:
approval-mozilla-aurora+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
yzen
:
review+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 2•8 years ago
|
||
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)
Keywords: regressionwindow-wanted
Comment 3•8 years ago
|
||
Is there a speculative fix we can do here?
Flags: needinfo?(surkov.alexander)
Comment 4•8 years ago
|
||
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?
Assignee | ||
Comment 5•8 years ago
|
||
(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)
Comment 6•8 years ago
|
||
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)
Updated•8 years ago
|
Priority: -- → P2
Assignee | ||
Comment 7•8 years ago
|
||
(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)
Comment 8•8 years ago
|
||
I'm not seeing reports for FF 53. Updating status flags accordingly.
Comment 9•8 years ago
|
||
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.
Group: core-security
status-firefox54:
--- → affected
Flags: needinfo?(dbolter)
Keywords: csectype-uaf
Comment 10•8 years ago
|
||
Alex can you take another look here?
Flags: needinfo?(dbolter) → needinfo?(surkov.alexander)
Comment 11•8 years ago
|
||
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
status-firefox-esr45:
--- → affected
tracking-firefox52:
--- → +
tracking-firefox53:
--- → +
tracking-firefox54:
--- → +
Keywords: sec-high
Assignee | ||
Comment 12•8 years ago
|
||
(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)
Assignee | ||
Updated•8 years ago
|
Keywords: steps-wanted,
testcase-wanted
Comment 13•8 years ago
|
||
(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)
Assignee | ||
Comment 14•8 years ago
|
||
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)
Comment 15•8 years ago
|
||
Adding Florin to the cc list to address the request for testing in Comment 14.
Reporter | ||
Comment 16•8 years ago
|
||
many of the recent user comments talk about emptying their junk folder in outlook/hotmail when the crash happened: http://bit.ly/2kUbIOC
Comment 17•8 years ago
|
||
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.
Updated•8 years ago
|
Flags: needinfo?(andrei.vaida)
Comment 18•8 years ago
|
||
(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.
Assignee | ||
Comment 19•8 years ago
|
||
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)
Updated•8 years ago
|
Flags: needinfo?(dbolter)
Comment 20•8 years ago
|
||
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+
Assignee | ||
Comment 21•8 years ago
|
||
(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.
Assignee | ||
Comment 22•8 years ago
|
||
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?
Comment 23•8 years ago
|
||
sec-approval+ for trunk. We'll want branch patches nominated as well after it goes into trunk.
tracking-firefox-esr45:
--- → 52+
Updated•8 years ago
|
Attachment #8835531 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 24•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/69404501d4d8fc26cc4317ccaec8cf997e7b599a
Bug 1321384 - attempt to fix a wrong index issue on accessible child removal, r=eeejay
Comment 25•8 years ago
|
||
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 26•8 years ago
|
||
Please request Aurora/Beta approval on this when you get a chance.
Assignee: nobody → surkov.alexander
Flags: needinfo?(surkov.alexander)
Assignee | ||
Comment 27•8 years ago
|
||
(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)
Assignee | ||
Comment 28•8 years ago
|
||
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 29•8 years ago
|
||
Comment on attachment 8835531 [details] [diff] [review]
patch
Fix a crash & sec-high. Aurora53+.
Attachment #8835531 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 30•8 years ago
|
||
Attachment #8835531 -
Flags: approval-mozilla-beta?
Comment 31•8 years ago
|
||
uplift |
Updated•8 years ago
|
Group: dom-core-security → core-security-release
Comment 32•8 years ago
|
||
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+
Comment 33•8 years ago
|
||
uplift |
Comment 34•8 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/5701c0b8b4b8
Did we want to get this on esr45 still?
Assignee | ||
Comment 35•8 years ago
|
||
(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)
Updated•8 years ago
|
Flags: needinfo?(andrei.vaida)
Comment 36•8 years ago
|
||
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.
Reporter | ||
Comment 37•8 years ago
|
||
there are still crashes with this signature in 52.0b7 - should we reopen the bug?
http://bit.ly/2m4mbFD
Flags: needinfo?(surkov.alexander)
Comment 38•8 years ago
|
||
Reopening preemptively; no apparent change in b7.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 39•8 years ago
|
||
(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)
Assignee | ||
Comment 40•8 years ago
|
||
it'd be interesting to see if there's a problem here as well
Attachment #8839586 -
Flags: review?(eitan)
Updated•8 years ago
|
Attachment #8839586 -
Flags: review?(eitan) → review+
Assignee | ||
Comment 41•8 years ago
|
||
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 42•8 years ago
|
||
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+
Assignee | ||
Comment 43•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/361f2f36838371c5c4e1d68571e0c69a77cc75b4
Bug 1321384 - add diagnostics to Accessible::Move, r=eeejay
Assignee | ||
Comment 44•8 years ago
|
||
(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
Comment 45•8 years ago
|
||
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 46•8 years ago
|
||
Assignee | ||
Comment 47•8 years ago
|
||
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+
Updated•8 years ago
|
Comment 49•8 years ago
|
||
uplift |
Assignee | ||
Comment 50•8 years ago
|
||
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
Assignee | ||
Comment 51•8 years ago
|
||
Attachment #8844898 -
Flags: review?(yzenevich)
Comment 52•8 years ago
|
||
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+
Assignee | ||
Comment 53•8 years ago
|
||
Attachment #8844898 -
Attachment is obsolete: true
Attachment #8844905 -
Flags: review?(yzenevich)
Assignee | ||
Comment 54•8 years ago
|
||
(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.
Assignee | ||
Comment 55•8 years ago
|
||
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 56•8 years ago
|
||
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+
Assignee | ||
Comment 57•8 years ago
|
||
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 58•8 years ago
|
||
Comment on attachment 8844999 [details] [diff] [review]
RemoveChild diagnostics
sec-approval+
Attachment #8844999 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 59•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/dea9f3038c4d1cad37ac5d9908dbaef42fabecf2
Bug 1321384 - add debuggin assert for Accessible::RemoveChild, r=yzen
https://hg.mozilla.org/mozilla-central/rev/dea9f3038c4d
Unsure where this leaves status flags, milestones, resolution.
Comment 61•8 years ago
|
||
Fwiw, this signature also shows up in EXCEPTION_STACK_OVERFLOW crashes:
bp-f4820ef9-35bd-44a4-ba38-c0ecf2170312
Comment 62•8 years ago
|
||
(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)
Assignee | ||
Comment 63•8 years ago
|
||
(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.
Assignee | ||
Updated•8 years ago
|
Blocks: treeupdatea11y
Alexander, does this updated patch still need to land for aurora 54 and beta 53?
Flags: needinfo?(surkov.alexander)
Comment 65•8 years ago
|
||
Also, do we need to uplift anything from bug 1346518 or other bugs mentioned there as crash causes?
Assignee | ||
Comment 66•8 years ago
|
||
(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)
Comment 67•8 years ago
|
||
ok, we'll want any beta updates this week or perhaps next, since we're at beta 6 currently
Comment 68•8 years ago
|
||
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)
Comment 69•8 years ago
|
||
Alex are there any low risk fixes or mitigations we should try to take now for 53?
Flags: needinfo?(dbolter)
Assignee | ||
Comment 70•8 years ago
|
||
(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)
Reporter | ||
Comment 72•8 years ago
|
||
(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
Comment 73•8 years ago
|
||
Are we going to take this on ESR52 and ESR45?
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Flags: needinfo?(jcristau)
Assignee | ||
Comment 74•8 years ago
|
||
(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)
Comment 76•8 years ago
|
||
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)
Comment 77•8 years ago
|
||
(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.
Updated•8 years ago
|
We are still seeing some crashes with this signature on 53.0 release.
Comment 80•8 years ago
|
||
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.
status-firefox55:
--- → fixed
tracking-firefox-esr45:
54+ → ---
Target Milestone: mozilla54 → mozilla55
Comment 81•8 years ago
|
||
Still seeing no 55 crashes. Still crashes in 54b4; waiting on a patch for bug 1342433 for 54 to land
Comment 82•7 years ago
|
||
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: 8 years ago → 7 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 83•7 years ago
|
||
appears to be fixed by bug 1363027 (landed 25th May) in Firefox 54, the last crash is dated by 25 (https://crash-stats.mozilla.com/signature/?product=Firefox&signature=mozilla%3A%3Aa11y%3A%3AAccessible%3A%3ASetARIAHidden&date=%3E%3D2017-05-23T16%3A33%3A00.000Z&date=%3C2017-05-30T16%3A33%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-build_id&_sort=-version&_sort=-date&page=1).
Blocks: brokentreea11y
Reporter | ||
Comment 84•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 85•7 years ago
|
||
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)
Comment 86•7 years ago
|
||
Agreed, we should close, fixed in 55. Alex can you prepare a patch for ESR 52 and request approval?
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Flags: needinfo?(dbolter)
Resolution: --- → FIXED
Assignee | ||
Comment 87•7 years ago
|
||
(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)
Comment 88•7 years ago
|
||
I would presume bug 1342433
Comment 89•7 years ago
|
||
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)
Assignee | ||
Comment 90•7 years ago
|
||
(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)
Comment 91•7 years ago
|
||
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)
Comment 92•7 years ago
|
||
Any updates on an ESR patch for this? We're running out of runway for the release.
Comment 93•7 years ago
|
||
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)
Assignee | ||
Comment 94•7 years ago
|
||
(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)
Comment 95•7 years ago
|
||
537 crashes on ESR 52.2.1 in the last week according to crash-stats.
Assignee | ||
Comment 96•7 years ago
|
||
(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?
Updated•7 years ago
|
Whiteboard: [adv-main55+]
Comment 97•7 years ago
|
||
We've uplifted bug 1363027 to ESR52 as well now. I think we can call this bug as finished as it'll ever be.
Updated•7 years ago
|
Whiteboard: [adv-main55+] → [adv-main55+][adv-esr52.3+]
Comment 98•7 years ago
|
||
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]
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•