571246 - Firefox crashes on Mapy.cz [@ nsCoreUtils::IsAncestorOf(nsINode*, nsINode*, nsINode*)]

Status: RESOLVED WORKSFORME

(Core :: Disability Access APIs, defect)

Description

[Pavel Cvrcek [:JasnaPaka]]

Mozilla/5.0 (Windows; U; Windows NT 6.0; cs; rv:1.9.3a5pre) Gecko/20100610 Minefield/3.7a5pre

1) Visit http://www.mapy.cz/#mm=ZP@sa=s@st=s@ssq=litice@sss=1@ssp=113826924_123416268_156753004_156626636@x=130596067@y=134626045@z=12
2) Press CTRL and try to select some area on the map by mouse.
3) Firefox crashes.

http://crash-stats.mozilla.com/report/index/bp-44b459be-9e10-42c9-ae59-baedb2100610

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hmm.  Do you have accessibility enabled on purpose?  If not, do you have something like Kaspersky anti-virus or a pen input device around?

The crash looks like a null deref due to aPossibleDescendantNode not being a descendant of aRootNode, so us walking off the end of the parent chain.  Presumably the changeAcc->GetParent() returns the accessible for some unrelated node for some reason in CreateTextChangeEventForNode.

Comment 2

[Pavel Cvrcek [:JasnaPaka]]

(In reply to comment #1)
> Hmm.  Do you have accessibility enabled on purpose?  If not, do you have
> something like Kaspersky anti-virus or a pen input device around?

No. 

I use Mapy.cz quite often so I think this problem is new. I can try older builds  if it helps you.

Comment 3

[alexander :surkov (:asurkov)]

I think Boris wonder how it happens that you have enabled accessibility.

This problem has been introduced recently so it doesn't make sense to try older builds.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> No. 

In that case, we should probably try to figure out why accessibility is on in your case at all.  :(

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 450382 [details] [diff] [review]
patch

Looks ok, assuming the parent accessible not being for the parent is ok...

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 450382 [details] [diff] [review]
patch

r=me. Yep that should fix the recent regression.

Comment 9

[[e-mail]]

@ matti
  I have a drawing tablet installed on my computer with the necessary tablet enhancements of vista. Maybe this caused accessibility to be turned on?

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #9)
> @ matti
>   I have a drawing tablet installed on my computer with the necessary tablet
> enhancements of vista. Maybe this caused accessibility to be turned on?

If it uses Windows Tablet PC software, which uses MSAA, yes that would invoke accessibility.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> I have a drawing tablet

That could well do it.  See comment 1 paragraph 1.  Still not sure why Pavel was hitting this, though...

Comment 12

[alexander :surkov (:asurkov)]

landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/5df144c4cfdf

Comment 13

[[e-mail]]

Should this have been fixed in

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.3a6pre) Gecko/20100611 Minefield/3.7a6pre ( .NET CLR 3.5.30729) ID:20100611035754

or do I have to wait till tonight's nightly for this patch?

Cause it's still crashing with this build
http://crash-stats.mozilla.com/report/index/a070b8f8-bfce-4204-b5cf-3ab6f2100611

According to this thread it's supposed to be in this build already:
http://forums.mozillazine.org/viewtopic.php?f=23&t=1919939

Comment 14

[Boris Zbarsky [:bzbarsky]]

Ezra, what's the build id of your build?  The one from about:buildconfig?

Comment 15

[[e-mail]]

about:buildconfig

Source

Built from http://hg.mozilla.org/mozilla-central/rev/431eab8cf6ab
Build platform
target
i686-pc-mingw32
Build tools
Compiler 	Version 	Compiler flags
cl 	14.00.50727.762 	-TC -nologo -W3 -Gy -Fdgenerated.pdb -DNDEBUG -DTRIMMED -Zi -Zi -UDEBUG -DNDEBUG -GL -wd4624 -wd4952 -O1
cl 	14.00.50727.762 	-GR- -TP -nologo -Zc:wchar_t- -W3 -Gy -Fdgenerated.pdb -wd4800 -DNDEBUG -DTRIMMED -Zi -Zi -UDEBUG -DNDEBUG -GL -wd4624 -wd4952 -O1
Configure arguments

--enable-application=browser --enable-update-channel=nightly --enable-update-packaging --enable-jemalloc --enable-tests

Comment 16

[Boris Zbarsky [:bzbarsky]]

That build has the patch that was pushed for this bug, yes.

Reopening per comment 13.

Comment 17

[alexander :surkov (:asurkov)]

Bosis, this is a document without docshell (http://hg.mozilla.org/mozilla-central/annotate/431eab8cf6ab/accessible/src/base/nsCoreUtils.cpp#l490). Btw, can the document without docshell be a root document or it's always contained within other document? Can the document that had docshell be without docshell?

Comment 18

[Boris Zbarsky [:bzbarsky]]

> Btw, can the document without docshell be a root document or it's always
> contained within other document?

What do you mean?

> Can the document that had docshell be without docshell?

In general, yes (though pagehide would happen first).

Comment 19

[Boris Zbarsky [:bzbarsky]]

I do see huge slews of accessibility asserts on the url in the url field, but no crash over here...  Those asserts look unhappy, though.

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #18)
> > Btw, can the document without docshell be a root document or it's always
> > contained within other document?
> 
> What do you mean?

In terms of accessibility root document is document having window (in terms of Windows UI). We think the document is root if it's hasn't docshell root.

> 
> > Can the document that had docshell be without docshell?
> 
> In general, yes (though pagehide would happen first).

Ok, it shouldn't be our case because we check nsIDocument::IsVisible() before we try to create document accessible.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to comment #19)
> I do see huge slews of accessibility asserts on the url in the url field, but
> no crash over here...  Those asserts look unhappy, though.

Unfortunately last months I've added amount of assertions for problems but I haven't chance to fix them yet. Most of them are related with process of accessible creation when something goes wrong.

Comment 22

[Boris Zbarsky [:bzbarsky]]

> In terms of accessibility root document is document having window (in terms of
> Windows UI).

A document with no docshell never has a window in this sense.

Comment 23

[alexander :surkov (:asurkov)]

If document hasn't docshell (like svg document) then is it content or chrome document or something else?

For example, nsCoreUtils::IsContentDocument() checks 

docShellTreeItem->GetItemType(&contentType);
contentType == nsIDocShellTreeItem::typeContent

Comment 24

[Boris Zbarsky [:bzbarsky]]

> then is it content or chrome document or something else?

Could be either.

If you want to specifically detect SVG resource documents and figure out what the associated docshell is, see https://hg.mozilla.org/mozilla-central/file/06ab72f852b3/content/base/public/nsIDocument.h#l1136

Comment 25

[alexander :surkov (:asurkov)]

Is SVG resource document visible on the screen or should we actually to work with nsIDocument::GetDisplayDocument() for it? What is SVG resource document about?

Comment 26

[Boris Zbarsky [:bzbarsky]]

> What is SVG resource document about?

In SVG you can do things like say "use this gradient to fill this shape", where "this gradient" is an SVG element... but this element need not be in the same document as the thing being filled.  In that case, the separate document needs to be loaded and parsed; then the gradient (or whatever other paint server was referenced) is used to paint the "main" document.

The user does not directly interact with the resource document in any way, scripting is disabled in the resource document, etc.  I can't tell you whether we create accessibles for things in resource documents...  You can try http://people.mozilla.org/~bzbarsky/svg-external-resource/example.html to see some of this stuff in action.

Comment 27

[alexander :surkov (:asurkov)]

I don't see accessible for resource documents anywhere in accessible tree. So we can create it I think since we don't require docshell for document accessible but I don't think it's discoverable by AT. If the user doesn't interact with resource document then it doesn't make sense to expose it to AT I think. What's the best way to ignore it? Check the docshell or nsIDocument::GetDisplayDocument()?

Comment 28

[Boris Zbarsky [:bzbarsky]]

The latter.  I agree that we don't want to expose them to AT.

Comment 29

[alexander :surkov (:asurkov)]

Attachment: svg res doc patch

Comment 30

[alexander :surkov (:asurkov)]

(In reply to comment #29)
> Created an attachment (id=451503) [details]
> svg res doc patch

looking at crash stack (http://crash-stats.mozilla.com/report/index/a070b8f8-bfce-4204-b5cf-3ab6f2100611) It doesn't look it's about svg resource documents. nsAccEventQueue::Flush() flushes the layout and we get notifications about style changes and try to get document accessible for the document containing changed content.

Boris, can flush layout be prepared for presshell that is about to destroy (in terms of nsIAccessibilityService::PresShellDestroyed from bug 571459)?

Ezra, can you provide steps to reproduce?

Comment 31

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 451503 [details] [diff] [review]
svg res doc patch

r=me. I don't see a reason to expose an svg resource doc either.

>+  // Ignore temporary, hiding documents and svg resource documents.

Nit: "Ignore temporary, hiding, and svg resource documents."

Comment 32

[Boris Zbarsky [:bzbarsky]]

> It doesn't look it's about svg resource documents.

Right.

> can flush layout be prepared for presshell that is about to destroy (in
> terms of nsIAccessibilityService::PresShellDestroyed from bug 571459)?

I'm not sure what you're asking....  But during the layout flush call, the presshell's Destroy method can be triggered.

But note comment 18 and our discussion earlier today about current code not seeing pagehide events sometimes (which the patch in bug 571459 fixes).

Comment 33

[alexander :surkov (:asurkov)]

(In reply to comment #32)

> > can flush layout be prepared for presshell that is about to destroy (in
> > terms of nsIAccessibilityService::PresShellDestroyed from bug 571459)?
> 
> I'm not sure what you're asking....  But during the layout flush call, the
> presshell's Destroy method can be triggered.

I thought whether PresShellDestroyed can help here but I was wrong because we crash when document accessible is created, i.e. more earlier shutdown of accessible document shouldn't help here.

It looks like we create doc accessible for a document what accessible was shut down already (or never created - though I doubt). So either PresShell::FlushPendingNotifications() for one document triggers nsFrameManager::ReResolveStyleContext() for other document or PresShell::FlushPendingNotifications() should trigger pagehide event. At least I don't see other options how the document accessible can be shutdown during its event processing.

So possibly the best way is to add check for docshell during document accessible creation.

Does nsCSSFrameConstructor::ProcessPendingRestyles make sense for document without docshell?

Comment 34

[Boris Zbarsky [:bzbarsky]]

> So either PresShell::FlushPendingNotifications() for one document triggers
> nsFrameManager::ReResolveStyleContext() for other document

That can easily happen.

> or PresShell::FlushPendingNotifications() should trigger pagehide event

I think that can also happen.  FlushPendingNotifications can run arbitrary script (and in fact can spin the event loop!).

But the point is, we're about to make that flush go away, right?

> Does nsCSSFrameConstructor::ProcessPendingRestyles make sense for document
> without docshell?

Not really (I think even for resource docs it really doesn't).

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 451503 [details] [diff] [review]
svg res doc patch

Looks fine.

Comment 36

[alexander :surkov (:asurkov)]

(In reply to comment #34)
> > So either PresShell::FlushPendingNotifications() for one document triggers
> > nsFrameManager::ReResolveStyleContext() for other document
> 
> That can easily happen.
> 
> > or PresShell::FlushPendingNotifications() should trigger pagehide event
> 
> I think that can also happen.  FlushPendingNotifications can run arbitrary
> script (and in fact can spin the event loop!).
> 
> But the point is, we're about to make that flush go away, right?

Right.

> > Does nsCSSFrameConstructor::ProcessPendingRestyles make sense for document
> > without docshell?
> 
> Not really (I think even for resource docs it really doesn't).

So if FlushPendingNotifications() is result of and document accessible docshell destruction and and further its processing is result of document accessible creation then we 
1) shouldn't try to create doc accessible without docshell
2) or FlushPendingNotifications() should be canceled somehow for documents with destroyed docshell.

Comment 37

[alexander :surkov (:asurkov)]

and document accessible docshell - > docshell and document accessible

Comment 39

[alexander :surkov (:asurkov)]

ignore svg resource document patch landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/6bd508f330f1

Comment 40

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This is far and away the #1 crash on 3.7a5 (by a factor of 5)

Comment 41

[alexander :surkov (:asurkov)]

no recent crashes, can't reproduce the bug by following steps from description (with a11y enabled), it seems the crash has gone, close as worksforme, please reopen it if you can reproduce the bug.