Last Comment Bug 862863 - inactive document accessible might be lost
: inactive document accessible might be lost
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- major (vote)
: mozilla23
Assigned To: alexander :surkov
:
: alexander :surkov
Mentors:
Depends on:
Blocks: treeupdatea11y
  Show dependency treegraph
 
Reported: 2013-04-17 08:53 PDT by alexander :surkov
Modified: 2013-07-15 08:06 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
tweak logging (1.72 KB, patch)
2013-04-17 19:35 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch (2.61 KB, patch)
2013-04-17 20:30 PDT, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description alexander :surkov 2013-04-17 08:53:09 PDT
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.
Comment 1 David Bolter [:davidb] 2013-04-17 08:56:18 PDT
Ugh.
Comment 2 alexander :surkov 2013-04-17 19:35:47 PDT
Created attachment 738862 [details] [diff] [review]
tweak logging

I introduced changes in Shutdown() to make logging work properly, if we null out mDocumentNode before RemoveChild then OuterDoc fails to log.
Comment 3 alexander :surkov 2013-04-17 20:30:34 PDT
Created attachment 738877 [details] [diff] [review]
patch

put the document into hanging documents instead shutdown
Comment 4 Marco Zehe (:MarcoZ) 2013-04-17 21:56:53 PDT
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.
Comment 5 alexander :surkov 2013-04-17 22:33:18 PDT
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 6 Trevor Saunders (:tbsaunde) 2013-05-03 11:11:58 PDT
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?
Comment 7 alexander :surkov 2013-05-03 17:40:14 PDT
Trev, what about logging part (curious if you missed it)?
Comment 8 Trevor Saunders (:tbsaunde) 2013-05-03 19:00:56 PDT
(In reply to alexander :surkov from comment #7)
> Trev, what about logging part (curious if you missed it)?

see comment 6?
Comment 9 alexander :surkov 2013-05-03 20:45:38 PDT
(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
Comment 10 Phil Ringnalda (:philor) 2013-05-04 14:04:41 PDT
https://hg.mozilla.org/mozilla-central/rev/a546ce4ccb14
Comment 11 Trevor Saunders (:tbsaunde) 2013-05-06 20:08:28 PDT
(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.
Comment 12 alexander :surkov 2013-05-06 20:24:11 PDT
(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.
Comment 13 Trevor Saunders (:tbsaunde) 2013-05-07 13:17:42 PDT
(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.
Comment 14 alexander :surkov 2013-05-07 19:05:55 PDT
I still don't follow why mDocumentNode is special so making it null in that place may be crucial.
Comment 15 Trevor Saunders (:tbsaunde) 2013-05-22 18:09:20 PDT
(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.
Comment 16 alexander :surkov 2013-06-13 04:19:14 PDT
Is a concern that you think that we may have a dependencies on mDocumentNode during shutdown phase which we use over defunct state?
Comment 17 alexander :surkov 2013-07-15 08:06:58 PDT
Comment on attachment 738862 [details] [diff] [review]
tweak logging

new home bug 893812 for this patch

Note You need to log in before you can comment on or make changes to this bug.