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

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Document load and busy events may be fired before subdocuments are ready. As well busy state flag is kept.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[Trevor Saunders (:tbsaunde)]

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.

Comment 3

[alexander :surkov (:asurkov)]

(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

Comment 4

[Trevor Saunders (:tbsaunde)]

(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

Comment 5

[alexander :surkov (:asurkov)]

(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 6

[Trevor Saunders (:tbsaunde)]

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.

Comment 7

[alexander :surkov (:asurkov)]

(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

Comment 8

[alexander :surkov (:asurkov)]

inbound land http://hg.mozilla.org/integration/mozilla-inbound/rev/acadcaa1c7e6

Comment 9

[alexander :surkov (:asurkov)]

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

in-testuite+ because blocks random oranges