document load and busy events may be fired before subdocuments are ready

RESOLVED FIXED in mozilla8

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

({access})

unspecified
mozilla8
access
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
Document load and busy events may be fired before subdocuments are ready. As well busy state flag is kept.
(Assignee)

Updated

6 years ago
Blocks: 660214
(Assignee)

Updated

6 years ago
Blocks: 660424
(Assignee)

Updated

6 years ago
Blocks: 562328
(Assignee)

Comment 1

6 years ago
Created attachment 551100 [details] [diff] [review]
patch
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #551100 - Flags: review?(trev.saunders)
Comment on attachment 551100 [details] [diff] [review]
patch


>-  PRUint32 childDocCount = mHangingChildDocuments.Length();
>-  for (PRUint32 idx = 0; idx < childDocCount; idx++) {
>+  PRUint32 hangingDocCnt = mHangingChildDocuments.Length();
>+  for (PRUint32 idx = 0; idx < hangingDocCnt; idx++) {

any reason for the rename to Cnt? I'm not sure what I think of using Cnt for count.

>+    if (childDocIdx == childDocCnt) {
>+      mDocument->ProcessLoad();
>+      if (!mDocument)
>+        return;

?!? I'm not really sure what you were  meaning to do here, I'd like to see a patch with this doing something sain I think.

>+    }
>+  }
>+
>   // Process only currently queued generic notifications.
>   nsTArray < nsRefPtr<Notification> > notifications;
>   notifications.SwapElements(mNotifications);
> 
>   PRUint32 notificationCount = notifications.Length();
>   for (PRUint32 idx = 0; idx < notificationCount; idx++) {
>     notifications[idx]->Process();
>     if (!mDocument)
>@@ -305,20 +319,22 @@ NotificationController::WillRefresh(mozi
>         if (showOrHideEvent->mTextChangeEvent)
>           mDocument->ProcessPendingEvent(showOrHideEvent->mTextChangeEvent);
>       }
>     }
>     if (!mDocument)
>       return;
>   }
> 
>-  // Stop further processing if there are no newly queued insertions,
>-  // notifications or events.
>+  // Stop further processing if there are no newly notifications of any kind or
>+  // events and document load is processed.

that phrasing is weird, newly -> new maybe?

>   if (mContentInsertions.Length() == 0 && mNotifications.Length() == 0 &&
>-      mEvents.Length() == 0 &&
>+      mEvents.Length() == 0 && mTextHash.Count() == 0 &&
>+      mHangingChildDocuments.Length() == 0 &&
>+      mDocument->HasLoadState(nsDocAccessible::eCompletelyLoaded) &&
>       mPresShell->RemoveRefreshObserver(this, Flush_Display)) {
>     mObservingState = eNotObservingRefresh;
>   }

that's a lot of things to test, also  I gues it makes sense to get rid of the braces.

I've taken a quick look through the rest of this, but  want to look more, which I can keep doing although probably tomorrow.
(Assignee)

Comment 3

6 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #2)

> any reason for the rename to Cnt? I'm not sure what I think of using Cnt for
> count.

no well motivated reason, just used cnt and liked it

> >+    if (childDocIdx == childDocCnt) {
> >+      mDocument->ProcessLoad();
> >+      if (!mDocument)
> >+        return;
> 
> ?!? I'm not really sure what you were  meaning to do here, I'd like to see a
> patch with this doing something sain I think.

I don't get your concern (did you mean sane instead sain?), could you ask concrete questions please?

> >   if (mContentInsertions.Length() == 0 && mNotifications.Length() == 0 &&
> >-      mEvents.Length() == 0 &&
> >+      mEvents.Length() == 0 && mTextHash.Count() == 0 &&
> >+      mHangingChildDocuments.Length() == 0 &&
> >+      mDocument->HasLoadState(nsDocAccessible::eCompletelyLoaded) &&
> >       mPresShell->RemoveRefreshObserver(this, Flush_Display)) {
> >     mObservingState = eNotObservingRefresh;
> >   }
> 
> that's a lot of things to test, also  I gues it makes sense to get rid of
> the braces.

when there's lot of statements under 'if' then I prefer to wrap if body by braces, it's easier to read
(In reply to alexander surkov from comment #3)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> 
> > any reason for the rename to Cnt? I'm not sure what I think of using Cnt for
> > count.
> 
> no well motivated reason, just used cnt and liked it

ok, was just sort of wondering if you want to use it more generally, or just a few cases like this one. (I sort of like it too)

> > >+    if (childDocIdx == childDocCnt) {
> > >+      mDocument->ProcessLoad();
> > >+      if (!mDocument)
> > >+        return;
> > 
> > ?!? I'm not really sure what you were  meaning to do here, I'd like to see a
> > patch with this doing something sain I think.
> 
> I don't get your concern (did you mean sane instead sain?), could you ask
> concrete questions please?

sure, sorry, I thought it would be obvious when reading it.  you dereference mDocument, and then you return if its null, but at that point you know its not NULL because you just dereferenced it so either we've crashed, or its not NULL.  We also check that mDocument isn't NULL at the begining of the function.

> > >   if (mContentInsertions.Length() == 0 && mNotifications.Length() == 0 &&
> > >-      mEvents.Length() == 0 &&
> > >+      mEvents.Length() == 0 && mTextHash.Count() == 0 &&
> > >+      mHangingChildDocuments.Length() == 0 &&
> > >+      mDocument->HasLoadState(nsDocAccessible::eCompletelyLoaded) &&
> > >       mPresShell->RemoveRefreshObserver(this, Flush_Display)) {
> > >     mObservingState = eNotObservingRefresh;
> > >   }
> > 
> > that's a lot of things to test, also  I gues it makes sense to get rid of
> > the braces.
> 
> when there's lot of statements under 'if' then I prefer to wrap if body by
> braces, it's easier to read

sure
(Assignee)

Comment 5

6 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> (In reply to alexander surkov from comment #3)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > 
> > > any reason for the rename to Cnt? I'm not sure what I think of using Cnt for
> > > count.
> > 
> > no well motivated reason, just used cnt and liked it
> 
> ok, was just sort of wondering if you want to use it more generally, or just
> a few cases like this one. (I sort of like it too)

we can use this wider (but this case is similar to case of 'idx' and 'index' naming, we use them both), I think I'm up to use common accepted shortenings for naming.

> > > >+    if (childDocIdx == childDocCnt) {
> > > >+      mDocument->ProcessLoad();
> > > >+      if (!mDocument)
> > > >+        return;
> > > 
> > > ?!? I'm not really sure what you were  meaning to do here, I'd like to see a
> > > patch with this doing something sain I think.
> > 
> > I don't get your concern (did you mean sane instead sain?), could you ask
> > concrete questions please?
> 
> sure, sorry, I thought it would be obvious when reading it.  you dereference
> mDocument, and then you return if its null, but at that point you know its
> not NULL because you just dereferenced it so either we've crashed, or its
> not NULL.  We also check that mDocument isn't NULL at the begining of the
> function.

You miss the case when object is destroyed when it's dereferenced, ProcessLoad fires events, so AT can do everything *before* ProcessLoad() returns.
Comment on attachment 551100 [details] [diff] [review]
patch


>+  mLoadState &= ~eDOMLoaded;

its a little tricky how this removes eCompletely loaded  implicitly.

>+  nsCOMPtr<nsISupports> container = mDocument->GetContainer();
>+  nsCOMPtr<nsIDocShellTreeItem> docShellTreeItem =
>+    do_QueryInterface(container);

why do we use the intermediary nsCOMPtr to nsISupports here? 

>+    // eDOMLoaded flag check is used for error pages as workaround to make this
>+    // method return correct result since error pages do not receive 'pageshow'
>+    // event and as consequence nsIDocument::IsShowing() returns false.

I know we've  been dealing with this in a11y for a while, but why don't we make nsIDocument::IsShowing do the right thing for this case?

>+    eTreeConstructed = 1 << 0,

nit, why not just use 1?

>   // nsDocAccessible
>+

nit any reason for this change?

>+  }
>+  void NotifyOfLoading(bool aIsReloading);
>+  friend class nsAccDocManager;
nit, I'd put a blank line after the brace, and after the method declaration.
Attachment #551100 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 7

6 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #6)
> Comment on attachment 551100 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> 
> >+  mLoadState &= ~eDOMLoaded;
> 
> its a little tricky how this removes eCompletely loaded  implicitly.

until you know what is eCompletelyLoaded - yeah

> >+  nsCOMPtr<nsISupports> container = mDocument->GetContainer();
> >+  nsCOMPtr<nsIDocShellTreeItem> docShellTreeItem =
> >+    do_QueryInterface(container);
> 
> why do we use the intermediary nsCOMPtr to nsISupports here? 

because getContainer() returns already_AddRefed<nsISupports>, you need to query and release it, two nsCOMPtr is explicit way to do that.

> >+    // eDOMLoaded flag check is used for error pages as workaround to make this
> >+    // method return correct result since error pages do not receive 'pageshow'
> >+    // event and as consequence nsIDocument::IsShowing() returns false.
> 
> I know we've  been dealing with this in a11y for a while, but why don't we
> make nsIDocument::IsShowing do the right thing for this case?

it's used to detect when selection change events should be ignored or not, we should process them when DOM document is ready, IsShowing() is a good way for that.

> >+    eTreeConstructed = 1 << 0,
> 
> nit, why not just use 1?

either case is fine with me, changed.

> >   // nsDocAccessible
> >+
> 
> nit any reason for this change?

> >+  }
> >+  void NotifyOfLoading(bool aIsReloading);
> >+  friend class nsAccDocManager;
> nit, I'd put a blank line after the brace, and after the method declaration.

ok
(Assignee)

Comment 8

6 years ago
inbound land http://hg.mozilla.org/integration/mozilla-inbound/rev/acadcaa1c7e6
Whiteboard: [inbound]
(Assignee)

Comment 9

6 years ago
landed http://hg.mozilla.org/mozilla-central/rev/acadcaa1c7e6

in-testuite+ because blocks random oranges
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Blocks: 676874
You need to log in before you can comment on or make changes to this bug.