inactive document accessible might be lost

RESOLVED FIXED in mozilla23

Status

()

Core
Disability Access APIs
--
major
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access})

unspecified
mozilla23
access
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 2

4 years ago
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.
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #738862 - Flags: review?(trev.saunders)
(Assignee)

Comment 3

4 years ago
Created attachment 738877 [details] [diff] [review]
patch

put the document into hanging documents instead shutdown
Attachment #738877 - Flags: review?(trev.saunders)
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.
(Assignee)

Comment 5

4 years ago
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?
Attachment #738877 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 7

4 years ago
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?
(Assignee)

Comment 9

4 years ago
(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
https://hg.mozilla.org/mozilla-central/rev/a546ce4ccb14
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
(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.
(Assignee)

Comment 12

4 years ago
(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.
(Assignee)

Comment 14

4 years ago
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.
(Assignee)

Comment 16

4 years ago
Is a concern that you think that we may have a dependencies on mDocumentNode during shutdown phase which we use over defunct state?
(Assignee)

Comment 17

4 years ago
Comment on attachment 738862 [details] [diff] [review]
tweak logging

new home bug 893812 for this patch
Attachment #738862 - Flags: review?(trev.saunders)
You need to log in before you can comment on or make changes to this bug.