If the document container (outerdoc) is recreated and if we are lucky enough then we get a sequance container removal container insertion hide processing of removal destroys the document If document is inactive (no user interactions, no mutations) then its doesn't get recreated.

Ugh.

I introduced changes in Shutdown() to make logging work properly, if we null out mDocumentNode before RemoveChild then OuterDoc fails to log.

put the document into hanging documents instead shutdown

Alex, is this a regression, or has this behavior been in Gecko for a longer period of time? I'm wondering if this could account for some reports I've been getting with JAWS losing document contents in Firefox 20 and above, but not before, and if this might be a regression from bug 767756.

It was for a long time. Jim from ZT caught it in Firefox 19. I've heard that JAWS loosing content mystically because of bug 767756. Of course it doesn't mean that this bug can't cause a JAWS problem.

Comment on attachment 738862 [details] [diff] [review] tweak logging > #ifdef A11Y_LOG >- if (logging::IsEnabled(logging::eDocDestroy)) >+ if (logging::IsEnabled(logging::eDocDestroy)) { > logging::DocDestroy("document shutdown", mDocumentNode, this); >+ logging::Stack(); >+ } > #endif that seems fine > > if (mNotificationController) { > mNotificationController->Shutdown(); > mNotificationController = nullptr; > } > > RemoveEventListeners(); > > // Mark the document as shutdown before AT is notified about the document > // removal from its container (valid for root documents on ATK and due to > // some reason for MSAA, refer to bug 757392 for details). > mStateFlags |= eIsDefunct; >- nsCOMPtr<nsIDocument> kungFuDeathGripDoc = mDocumentNode; >- mDocumentNode = nullptr; this seems really unsafe, in particular we'll call out into screen readers and stuff before we stop pointing at mDocumentNode which I'm will to guess can cause use after free bugs. It also seems totally unrelated is it just debugging cruft?

Trev, what about logging part (curious if you missed it)?

(In reply to alexander :surkov from comment #7) > Trev, what about logging part (curious if you missed it)? see comment 6?

(In reply to Trevor Saunders (:tbsaunde) from comment #6) > > // Mark the document as shutdown before AT is notified about the document > > // removal from its container (valid for root documents on ATK and due to > > // some reason for MSAA, refer to bug 757392 for details). > > mStateFlags |= eIsDefunct; > >- nsCOMPtr<nsIDocument> kungFuDeathGripDoc = mDocumentNode; > >- mDocumentNode = nullptr; > > this seems really unsafe, in particular we'll call out into screen readers > and stuff before we stop pointing at mDocumentNode which I'm will to guess > can cause use after free bugs. as long as we mark the object as defuct I don't see any problems calling anyone. Do I miss something? > It also seems totally unrelated is it just > debugging cruft? It is because logging is not so useful without document node

(In reply to alexander :surkov from comment #9) > (In reply to Trevor Saunders (:tbsaunde) from comment #6) > > > > // Mark the document as shutdown before AT is notified about the document > > > // removal from its container (valid for root documents on ATK and due to > > > // some reason for MSAA, refer to bug 757392 for details). > > > mStateFlags |= eIsDefunct; > > >- nsCOMPtr<nsIDocument> kungFuDeathGripDoc = mDocumentNode; > > >- mDocumentNode = nullptr; > > > > this seems really unsafe, in particular we'll call out into screen readers > > and stuff before we stop pointing at mDocumentNode which I'm will to guess > > can cause use after free bugs. > > as long as we mark the object as defuct I don't see any problems calling > anyone. Do I miss something? maybe its technically safe, but I'd need audit a bunch of stuff to be absolutely sure of that, and I'd still worry about things that could break it in the future. > > It also seems totally unrelated is it just > > debugging cruft? > > It is because logging is not so useful without document node What exactly can get logged where it would be useful and safe? It seems like not much can call into it after that point.

(In reply to Trevor Saunders (:tbsaunde) from comment #11) > > as long as we mark the object as defuct I don't see any problems calling > > anyone. Do I miss something? > > maybe its technically safe, but I'd need audit a bunch of stuff to be > absolutely sure of that, and I'd still worry about things that could break > it in the future. I'd argue that it makes code cleaner since we don't need kungfu death grip comptrs. But basically what you said means that we have a hidden problems somewhere, it's a chance to discover and fix them. > > > It also seems totally unrelated is it just > > > debugging cruft? > > > > It is because logging is not so useful without document node > > What exactly can get logged where it would be useful and safe? It seems > like not much can call into it after that point. outdoc accessible logging that happens on mParent->RemoveChild(this) in DocAccessible::Shutdown. If you want I can file a separate bug on it to make the change discoverable.

(In reply to alexander :surkov from comment #12) > (In reply to Trevor Saunders (:tbsaunde) from comment #11) > > > > as long as we mark the object as defuct I don't see any problems calling > > > anyone. Do I miss something? > > > > maybe its technically safe, but I'd need audit a bunch of stuff to be > > absolutely sure of that, and I'd still worry about things that could break > > it in the future. > > I'd argue that it makes code cleaner since we don't need kungfu death grip > comptrs. But basically what you said means that we have a hidden problems > somewhere, it's a chance to discover and fix them. not really, they'll crash dereferencing the null mDocumentNode today if you make that change they maybe crash or maybe just do strange things that might be exploitable. > > > > It also seems totally unrelated is it just > > > > debugging cruft? > > > > > > It is because logging is not so useful without document node > > > > What exactly can get logged where it would be useful and safe? It seems > > like not much can call into it after that point. > > outdoc accessible logging that happens on mParent->RemoveChild(this) in > DocAccessible::Shutdown. > > If you want I can file a separate bug on it to make the change discoverable. that might make sense, but I'm not convinced we should change at all.

I still don't follow why mDocumentNode is special so making it null in that place may be crucial.

(In reply to alexander :surkov from comment #14) > I still don't follow why mDocumentNode is special so making it null in that > place may be crucial. I don't think it is particularly special, but I don't see why that matters.

Is a concern that you think that we may have a dependencies on mDocumentNode during shutdown phase which we use over defunct state?

Comment on attachment 738862 [details] [diff] [review] tweak logging new home bug 893812 for this patch