Closed Bug 900835 Opened 7 years ago Closed 7 years ago

crash in nsEventStateManager::PreHandleEvent(nsPresContext*, nsEvent*, nsIFrame*, nsEventStatus*)

Categories

(Core :: Disability Access APIs, defect, critical)

x86
Windows 7
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla26
Tracking Status
firefox25 --- verified
firefox26 --- verified

People

(Reporter: Jamie, Assigned: surkov)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

I've had quite a few of these crashes while programmatically clicking accessibles in the recent/saved searches area of twitter.com.

Steps to reproduce:
1. Ensure you have at least one recent or saved search on twitter.com.
2. Open twitter.com.
3. Press / to search.
4. Due to a bug, you'll need to press shift+tab and then tab to make recent/saved searches appear.
To reproduce generally:
5. Retrieve the accessible for a link to one of the searches.
6. Programmatically click the accessible.
To reproduce with NVDA:
5. Use object navigation to move to one of the search links.
6. Press NVDA+numpadEnter (laptop layout: NVDA+enter) to activate it.

bp-b53251e3-1b29-4f9e-a3d6-6ac082130801 .
bp-37e53cb1-6eae-4687-9f69-175212130802

Could this be a regression from the first patch of bug 894485? I doubt it, but thought it worth mentioning. Also, this may not be strictly a11y related, so my apologies if I've filed this in the wrong place.
Crash Signature: [@ nsEventStateManager::PreHandleEvent(nsPresContext*, nsEvent*, nsIFrame*, nsEventStatus*)] → [@ nsEventStateManager::PreHandleEvent(nsPresContext*, nsEvent*, nsIFrame*, nsEventStatus*) ]
Olli, ideas? It looks like mPresContext->EventStateManager() returns null
Hmm, why you think mPresContext->EventStateManager() returns null?

However, EventStateManager() is wrongly named. It may return null after unlink. But we're
not very consistent with naming stuff which may return null after unlink.
(In reply to Olli Pettay [:smaug] from comment #2)
> Hmm, why you think mPresContext->EventStateManager() returns null?

probably I misread it but it looked like manager must be a null:
http://hg.mozilla.org/mozilla-central/annotate/05d3797276d3/layout/base/nsPresShell.cpp#l6810
Yeah, possibly. Though the crash address is somewhat odd then. I'd expect something closer
to 0x0 crash address (some offset from null).
(In reply to Olli Pettay [:smaug] from comment #2)

> However, EventStateManager() is wrongly named. It may return null after
> unlink. But we're
> not very consistent with naming stuff which may return null after unlink.

Btw, Accessible::DoCommand starts a runnable so I guess when runnable triggers the unlink may happen
Attached patch patchSplinter Review
patch per irc chat with Olli
Assignee: nobody → surkov.alexander
Attachment #785007 - Flags: review?(trev.saunders)
Attachment #785007 - Flags: review?(bugs)
Comment on attachment 785007 [details] [diff] [review]
patch

It is not clear to me whether we want all those events if frame dies, but that
issue is not about this bug.
Attachment #785007 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #7)
> Comment on attachment 785007 [details] [diff] [review]
> patch
> 
> It is not clear to me whether we want all those events if frame dies, but
> that
> issue is not about this bug.

presumably they wont' be dispatched so result should be the same in the end, and we shouldn't run into this case often
Comment on attachment 785007 [details] [diff] [review]
patch

>+  nsCOMPtr<nsPresContext> presContext = presShell->GetPresContext();

shouldn't that be nsRefPtr?

>-  nsPresContext* presContext = presShell->GetPresContext();
>+  nsCOMPtr<nsPresContext> presContext = presShell->GetPresContext();

same, its not entirely clear to me we care if its alive after events, but might as well be safe
Attachment #785007 - Flags: review?(trev.saunders) → review+
ok, no smart pointers for prescontext
You should keep prescontext alive, so smart pointer please.
But yeah, refptr is technically correct with it.
(In reply to Olli Pettay [:smaug] from comment #11)
> You should keep prescontext alive, so smart pointer please.
> But yeah, refptr is technically correct with it.

ok then :)
https://hg.mozilla.org/mozilla-central/rev/4f36a65d85e7
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment on attachment 785007 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): not sure but the bug was introduced in Firefox 25 frame
User impact if declined: crashes (ping smaug for more info)
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): not risky
String or IDL/UUID changes made by this patch: no
Attachment #785007 - Flags: approval-mozilla-aurora?
Comment on attachment 785007 [details] [diff] [review]
patch

Very early in the Aurora cycle, might as well fix.
Attachment #785007 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: checkin-needed
(In reply to James Teh [:Jamie] from comment #0)
> 5. Retrieve the accessible for a link to one of the searches.
> 6. Programmatically click the accessible.
I don't really understand these steps. Can you please explain a bit ?
I'm trying to reproduce the problem first, and then to verify it on fixed branches.
Flags: needinfo?(jamie)
Verified fixed in Firefox 26.0a1 (2013-09-12).

I don't know how to test this without use of NVDA (a screen reader). I guess you could do it with a straight Python shell and NVDA's oleacc bindings, but the steps to find the required accessible would be ridiculously complicated. I can try to explain further how i tested it with NVDA, but you'd probably need to spend some time learning how to use NVDA to test it effectively.

Alex, is there a way to test this within Firefox (perhaps via the Web Console) that doesn't require an external AT?
Flags: needinfo?(jamie)
Status: RESOLVED → VERIFIED
(In reply to James Teh [:Jamie] from comment #19)
> Verified fixed in Firefox 26.0a1 (2013-09-12).

Thanks for your help James. Could you please also verify this is fixed with the latest Firefox 25 Beta:
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/25.0b2-candidates/build1/
Verified fixed in Firefox 25b2 candidate build 1.

It's worth noting that I've just realised Twitter have changed their interface a bit, so it's possible this isn't triggering exactly the same code path any more. However, it hasn't changed *that* drastically, so it's very likely it does.
You need to log in before you can comment on or make changes to this bug.