Closed Bug 1454572 Opened 6 years ago Closed 6 years ago

Crash when exiting DOM full screen with JAWS

Categories

(Core :: Disability Access APIs, defect)

All
Windows 10
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- wontfix
firefox60 --- verified
firefox61 --- verified

People

(Reporter: Jamie, Assigned: Jamie)

References

(Depends on 1 open bug)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

bp-48ed936c-65cd-44b8-a25e-7929a0180417

STR (with JAWS):
1. Open this URL: https://developer.mozilla.org/samples/domref/fullscreen.html
2. Ensure the document is focused.
3. Press JAWSKey+3 to pass key through, then enter. This switches the page to full screen.
4. Again, press JAWSKey+3 then enter to exit full screen.
Crash!

Important details about the crash:
Crash reason: Trying to collect rules for a detached pseudo-element
Some a11y event gets fired and JAWS then calls mozilla::a11y::sdnAccessible::get_computedStyleForProperties (accessible/windows/sdn/sdnAccessible.cpp:280). This then calls into style code, where we crash.

Bug 1411008 comment 4 suggests this might happen if we're dealing with "out-of-doc content". So, I debugged this with a local build, and indeed, mStateFlags was eIsNotInDocument... but not eIsDefunct.

Alex, how could an accessible not be in a document but not be shut down when handling an event?
DocAccessible::UnbindFromDocument does set eIsNotInDocument, but it also shuts down the accessible before returning.
The only other place I can see that sets this flag is DocAccessible::UncacheChildrenInSubtree. That one doesn't seme to shut down the child. But when would this happen without a subsequent shutdown? is it valid for an accessible to be in this state?
If it is valid, we should add a !IsInDocument() check to sdnAccessible::get_computedStyleForProperties.
Flags: needinfo?(surkov.alexander)
I was also able to reproduce this with Firefox 60.0b13: bp-1490985a-9f02-4af0-ab90-db4520180417
Crash Signature: [@ static void std::panicking::rust_panic_with_hook] → [@ static void std::panicking::rust_panic_with_hook] [@ core::option::expect_failed | style::style_resolver::StyleResolverForElement<T>::match_primary<T> ]
(In reply to James Teh [:Jamie] from comment #0)
> Alex, how could an accessible not be in a document but not be shut down when
> handling an event?
> DocAccessible::UnbindFromDocument does set eIsNotInDocument, but it also
> shuts down the accessible before returning.
> The only other place I can see that sets this flag is
> DocAccessible::UncacheChildrenInSubtree. That one doesn't seme to shut down
> the child. But when would this happen without a subsequent shutdown? is it
> valid for an accessible to be in this state?

Spontaneous idea: aria-hidden tree (which we still ddon't prune, but keep around somewhere). Could that be it?
(In reply to James Teh [:Jamie] from comment #0)

> Alex, how could an accessible not be in a document but not be shut down when
> handling an event?
> DocAccessible::UnbindFromDocument does set eIsNotInDocument, but it also
> shuts down the accessible before returning.
> The only other place I can see that sets this flag is
> DocAccessible::UncacheChildrenInSubtree. That one doesn't seme to shut down
> the child. But when would this happen without a subsequent shutdown? is it
> valid for an accessible to be in this state?

Here's the thing. When accessible gets hidden, then we do this:
* detach accessible from a document (in  DocAccessible::UncacheChildrenInSubtree as you correctly noticed)
* fire hide event
* shutdown the accessible

This design is for AT who listens events in-process and needs to fetch extra information about accessible object before it gets shutdown. Given the asynchronous design we live in today, this logic may be fairly out of date. If it's valid statement, then we have great opportunity to simplify our logic.

> If it is valid, we should add a !IsInDocument() check to
> sdnAccessible::get_computedStyleForProperties.

This will definitely fix the crash. However I'd say nsComputedDOMStyle::GetPropertyValue shouldn't crash in the first place. Daniel, correct?
Flags: needinfo?(surkov.alexander) → needinfo?(dholbert)
(In reply to alexander :surkov from comment #3)
> This will definitely fix the crash. However I'd say
> nsComputedDOMStyle::GetPropertyValue shouldn't crash in the first place.
> Daniel, correct?

Sure - if we have content/steps that makes us crash in nsComputedDOMStyle::GetPropertyValue, that's a style system bug, & please file! Thanks.
Flags: needinfo?(dholbert)
Oh, I misunderstood:
 (1) maybe might want to fix that style system bug directly here (instead of hacking around it in a11y & spinning off a followup -- though it seems like that's an option too)
 (2) this isn't for content-JS calling nsComputedDOMStyle::GetPropertyValue, but rather a11y code calling into it, e.g. via the backtrace shown in crash report bp-48ed936c-65cd-44b8-a25e-7929a0180417 

Punting needinfo to xidorn since he knows details around fullscreen & the rust code that's crashing here better than I do.
Flags: needinfo?(xidorn+moz)
(In reply to Marco Zehe (:MarcoZ) from comment #2)
> Spontaneous idea: aria-hidden tree (which we still ddon't prune, but keep
> around somewhere). Could that be it?

Right now, if I understand correctly, we don't do anything special at all for aria-hidden trees except apply the hidden attribute. They still remain attached to the rest of the tree at all times.
(In reply to alexander :surkov from comment #3)
> This design is for AT who listens events in-process and needs to fetch extra
> information about accessible object before it gets shutdown. Given the
> asynchronous design we live in today, this logic may be fairly out of date.

What sort of info can an AT even fetch when an accessible is in this state? Can it retrieve the parent?
Do you know if there were specific AT which needed this behaviour? I might check in with VFO about this.

Because objects have unique ids, an AT can just use the id to keep any local cache up to date. This might be a bit trickier in cases where there is no cache, though; e.g. perhaps live region removals.

But you're right: in an e10s world, this isn't going to work properly anyway. We do fire text changed events synchronously from content, but not hide events. So, one way or another, we need to re-evaluate this.
GetPropertyValue is called with a pseudo-element which has been detached from its originating element (rather than just a out-of-doc content node). The style system is not able to handle this case as described in bug 1411008 comment 4, since there is no rule can be applied to a pseudo-element with no originating element.

I haven't tried to reproduce this (because I don't have JAWS), but I would guess that a fullscreen change triggers this probably because there are some frame reconstruction happening on the video controls, which contain some element-based pseudo-elements like ::-moz-progress-bar and ::-moz-range-*.

If that's the case, this should be reproducible without fullscreen. You just need to trigger frame reconstruction via, e.g. repeatedly switching the display value of <video> between inline and block.

I don't have idea what should happen here. I suppose the accessible tree shouldn't hold a detached pseudo-element or pass it into the style system. But maybe we can also make the style system more tolerant to this case when internal code invoking it with a detached pseudo-element (e.g. by making that assertion a debug_assert instead).

I'm not sure how emilio think about that.
Flags: needinfo?(xidorn+moz)
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #8)
> I don't have idea what should happen here. I suppose the accessible tree
> shouldn't hold a detached pseudo-element or pass it into the style system.
> But maybe we can also make the style system more tolerant to this case when
> internal code invoking it with a detached pseudo-element (e.g. by making
> that assertion a debug_assert instead).

If this really is an invalid input as far as style is concerned, I'd personally rather not have a11y hitting an assertion we expect to hit, since an assertion really should be unexpected. In that case, I think I'd prefer a11y to just protect against it (as the client facing code). Alex, thoughts?
We can probably also protect it inside getComputedStyle. I'm not sure whether chrome script can get a reference of those pseudo-elements. If they can, we would probably want a protection there anyway.
Instead of wallpapering on the Rust side I'd rather do it on nsComputedDOMStyle with something like:

  if (mContent->IsInNativeAnonymousSubtree() && !mContent->IsInComposedDoc()) {
    aRv.Throw(NS_ERROR_UNEXPECTED);
    return;
  }

Or something like that.
(In reply to James Teh [:Jamie] from comment #7)
> (In reply to alexander :surkov from comment #3)
> > This design is for AT who listens events in-process and needs to fetch extra
> > information about accessible object before it gets shutdown. Given the
> > asynchronous design we live in today, this logic may be fairly out of date.
> 
> What sort of info can an AT even fetch when an accessible is in this state?
> Can it retrieve the parent?

All methods should work but parent, since it's unattached. I guess other things may get broken though because of this fact.

> Do you know if there were specific AT which needed this behaviour? I might
> check in with VFO about this.

iirc they were consumers of this behavior, so please

> Because objects have unique ids, an AT can just use the id to keep any local
> cache up to date. This might be a bit trickier in cases where there is no
> cache, though; e.g. perhaps live region removals.

You're right but it requires extra support on AT side for sure, so it all depends on their internal logic. If it allows that, then yes, if no then no.

> But you're right: in an e10s world, this isn't going to work properly
> anyway. We do fire text changed events synchronously from content, but not
> hide events. So, one way or another, we need to re-evaluate this.

I filed bug 1455135 on this
(In reply to James Teh [:Jamie] from comment #9)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #8)
> > I don't have idea what should happen here. I suppose the accessible tree
> > shouldn't hold a detached pseudo-element or pass it into the style system.
> > But maybe we can also make the style system more tolerant to this case when
> > internal code invoking it with a detached pseudo-element (e.g. by making
> > that assertion a debug_assert instead).
> 
> If this really is an invalid input as far as style is concerned, I'd
> personally rather not have a11y hitting an assertion we expect to hit, since
> an assertion really should be unexpected. In that case, I think I'd prefer
> a11y to just protect against it (as the client facing code). Alex, thoughts?

a11y may indeed to keep a strong reference to a content associated with a pseudo style (like CSS after or before) longer than that content may be expected to live. If this scenario is not considered valid, then we should probably come with some checks on a11y side.

The longer term solution for this would be bug 1455135, the short term one is eIsInDocument check as you suggested above.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #11)
> Instead of wallpapering on the Rust side I'd rather do it on
> nsComputedDOMStyle

Would you want this to assert? If it's going to assert, I'll check it in the a11y code, as we don't want this asserting for something we expect with certain clients. Otherwise, I'll submit a patch as you suggest.

(In reply to alexander :surkov from comment #13)
> The longer term solution for this would be bug 1455135, the short term one
> is eIsInDocument check as you suggested above.

Actually, we can't use eIsNotInDocument, as sdn nodes don't always correspond to Accessibles and thus we lazily fetch the accessible. I think I'd have to use code similar to that suggested in comment 11.
Flags: needinfo?(emilio)
Emilio, I went ahead and submitted the layout patch you suggested in comment 11. However, if you do think this needs an assert (comment 14), I'll switch strategy and patch a11y instead.
Comment on attachment 8969172 [details]
Bug 1454572: nsComputedDOMStyle: Don't crash when used on a detached pseudo-element. :emilio

https://reviewboard.mozilla.org/r/237908/#review243632

::: layout/style/nsComputedDOMStyle.cpp:1003
(Diff revision 1)
>  already_AddRefed<CSSValue>
>  nsComputedDOMStyle::GetPropertyCSSValueWithoutWarning(
>    const nsAString& aPropertyName,
>    ErrorResult& aRv)
>  {
> +  if (mContent->IsInNativeAnonymousSubtree() && !mContent->IsInComposedDoc()) {

This looks fine, but needs a comment, and needs to go into UpdateCurrentStyleSources, after we've done FlushPendingNotifications and such. That is because `FlushPendingNotifications` can kill the pseudo-element.

In particular, should go after the:

````
if (!mPresShell || !mPresShell->GetPresContext()) {
  ClearComputedStyle();
  return;
}
```

condition. And should be very similar to ^.

That would throw `NS_ERROR_NOT_AVAILABLE` in GetPropertyValue() instead via the null-check, but that seems fine, the exact error doesn't look very important.

The comment should look like:

```
// Normal web content can't access NAC, but Accessibility, DevTools and Editor 
// use this same API and anonymous content may end up here.
//
// Computing the style of a pseudo-element that doesn't have a parent doesn't
// really make sense.
```

Or something of the sort.
Attachment #8969172 - Flags: review?(emilio)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #17)
> In particular, should go after the:
> 
> ````
> if (!mPresShell || !mPresShell->GetPresContext()) {
>   ClearComputedStyle();
>   return;
> }
> ```
> 
> condition. And should be very similar to ^.

Actually I lied, should be a check in DoGetComputedStyleNoFlush. The end result is the same but covers more native callers.

I odon't think this needs to be an assertion, given it can trivially get broken. An assertion that we're chrome callers would be helpful I guess, but seems very non-trivial.

Thanks for working on this!
Flags: needinfo?(emilio)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #18)
> Thanks for working on this!

You're welcome, although I don't really have a clue what I'm doing when it comes to layout and I'm essentially just stealing your patch. :P

Updated patch coming.
Comment on attachment 8969172 [details]
Bug 1454572: nsComputedDOMStyle: Don't crash when used on a detached pseudo-element. :emilio

https://reviewboard.mozilla.org/r/237908/#review244010

r=me, though given I suggested the patch probably Xidorn should skim over it too.

::: layout/style/nsComputedDOMStyle.cpp:505
(Diff revision 2)
>                                                nsIPresShell* aPresShell,
>                                                StyleType aStyleType)
>  {
>    MOZ_ASSERT(aElement, "NULL element");
> +
> +  if (aElement->IsInNativeAnonymousSubtree() && !aElement->IsInComposedDoc()) {

nit: I'd maybe move this down after the `pseudoType` check, since it's (loosely) related.

Other than that this patch looks great, thank you!
Attachment #8969172 - Flags: review?(emilio) → review+
Comment on attachment 8969172 [details]
Bug 1454572: nsComputedDOMStyle: Don't crash when used on a detached pseudo-element. :emilio

Xidorn, mind sanity-checking? Thanks!
Attachment #8969172 - Flags: review?(xidorn+moz)
Comment on attachment 8969172 [details]
Bug 1454572: nsComputedDOMStyle: Don't crash when used on a detached pseudo-element. :emilio

https://reviewboard.mozilla.org/r/237908/#review244014

It looks good to me.
Attachment #8969172 - Flags: review?(xidorn+moz) → review+
Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0fcbe086a09b
nsComputedDOMStyle: Don't crash when used on a detached pseudo-element. r=emilio,xidorn:emilio
https://hg.mozilla.org/mozilla-central/rev/0fcbe086a09b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee: nobody → jteh
Crash Signature: [@ static void std::panicking::rust_panic_with_hook] [@ core::option::expect_failed | style::style_resolver::StyleResolverForElement<T>::match_primary<T> ] → [@ static void std::panicking::rust_panic_with_hook] [@ core::option::expect_failed | style::style_resolver::StyleResolverForElement<T>::match_primary<T> ] [@ style::style_resolver::StyleResolverForElement<T>::match_primary<T> ]
Verified fixed in: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0 ID:20180422100125
Comment on attachment 8969172 [details]
Bug 1454572: nsComputedDOMStyle: Don't crash when used on a detached pseudo-element. :emilio

Approval Request Comment
[Feature/Bug causing the regression]: Stylo.
[User impact if declined]: Crash when exiting DOM full screen (e.g. full screen videos) while running the JAWS screen reader.
[Is this code covered by automated tests?]: No; no mechanism for testing with third party accessibility products.
[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]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Simple check and return early for invalid state.
[String changes made/needed]: None.
Attachment #8969172 - Flags: approval-mozilla-beta?
Comment on attachment 8969172 [details]
Bug 1454572: nsComputedDOMStyle: Don't crash when used on a detached pseudo-element. :emilio

Add an early return to fix a crash. Approved for 60.0b15.
Attachment #8969172 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Crash Signature: [@ static void std::panicking::rust_panic_with_hook] [@ core::option::expect_failed | style::style_resolver::StyleResolverForElement<T>::match_primary<T> ] [@ style::style_resolver::StyleResolverForElement<T>::match_primary<T> ] → [@ static void std::panicking::rust_panic_with_hook] [@ core::option::expect_failed | style::style_resolver::StyleResolverForElement<T>::match_primary<T> ] [@ style::style_resolver::StyleResolverForElement<T>::match_primary<T> ] [@ static void core::op…
Flags: qe-verify+
I reproduced the initial issue using JAWS 2018.1804.26.400 and 60.0b14 build2 (20180419200216), on the site mentioned in comment 0 and also on other ones (e.g. https://www.youtube.com/; https://edition.cnn.com/). I have to mention that I didn't manage to switch the fullscreen using JAWSKey+3, but the crash (@ core::option::expect_failed | style::style_resolver::StyleResolverForElement<T>::match_primary<T>) was successfully triggered using the dedicated fullscreen button, too. 
I can confirm that 60.0b16 build1 (20180426170554) is verified fixed, using Windows 10 x64 and Windows 7 x64.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1460223
You need to log in before you can comment on or make changes to this bug.