Last Comment Bug 676213 - document load and busy events may be fired before subdocuments are ready
: document load and busy events may be fired before subdocuments are ready
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: alexander :surkov
:
:
Mentors:
Depends on:
Blocks: 562328 660214 660424 676874
  Show dependency treegraph
 
Reported: 2011-08-03 04:48 PDT by alexander :surkov
Modified: 2011-08-08 09:40 PDT (History)
2 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (29.80 KB, patch)
2011-08-05 11:26 PDT, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description alexander :surkov 2011-08-03 04:48:29 PDT
Document load and busy events may be fired before subdocuments are ready. As well busy state flag is kept.
Comment 1 alexander :surkov 2011-08-05 11:26:28 PDT
Created attachment 551100 [details] [diff] [review]
patch
Comment 2 Trevor Saunders (:tbsaunde) 2011-08-05 22:22:46 PDT
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 2011-08-07 20:08:53 PDT
(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) 2011-08-07 20:19:58 PDT
(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 2011-08-07 20:51:42 PDT
(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) 2011-08-07 23:30:09 PDT
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 2011-08-08 00:14:01 PDT
(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 2011-08-08 00:57:58 PDT
inbound land http://hg.mozilla.org/integration/mozilla-inbound/rev/acadcaa1c7e6
Comment 9 alexander :surkov 2011-08-08 05:41:31 PDT
landed http://hg.mozilla.org/mozilla-central/rev/acadcaa1c7e6

in-testuite+ because blocks random oranges

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