498015 - update accessible tree on content insertion after layout

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P1)

Description

[Mats Palmgren (inactive)]

STEPS TO REPRODUCE
1. start a debug- and a11y-enabled Firefox
2. click the History menu
3. move the mouse over the recent page items a few times

ACTUAL RESULTS
###!!! ASSERTION: Can only call this on frames that have been reflowed: '!(GetStateBits() & NS_FRAME_FIRST_REFLOW)', file layout/generic/nsTextFrameThebes.cpp, line 2075

PLATFORMS AND BUILDS TESTED
Bug occurs in Firefox trunk on Linux (x86-64)
Bug occurs in Firefox 1.9.1 branch on Linux (x86-64)

Comment 1

[Mats Palmgren (inactive)]

Attachment: stack

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Yeah we need to call FlushPendingNotifications I think. I'll try to get to this one. What is the severity?

Comment 3

[alexander :surkov (:asurkov)]

I'm not sure it affect on screen readers users but it brings a pain to debug on windows:)

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Roc, can you take a quick look at the stack Mats provides in comment #1 and see if the solution is painfully obvious to you?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

nsAccessNode::GetFrame needs to flush at least up to Flush_Style before calling GetPrimaryFrame, otherwise the frame might not be created yet.

Somewhere between getting the frame and calling GetRenderedText, you should Flush_Reflow as well. You could do that in GetFrame, although then if there are places that call GetFrame and don't need an up-to-date layout, you could be wasting work.

Comment 6

[alexander :surkov (:asurkov)]

After bug 570275 when we started to update the tree when nsCSSFrameConstructor::AppendContent/ContentRangeInserted/ContentRemoved I see lot of these assertions. When we update the accessible tree, we may create a11y text change events what calls GetRenderedText, and then GetTrimmedOffsets which asserts.

Is there a safe way to get rendered text when we create text change events?

Comment 7

[Boris Zbarsky [:bzbarsky]]

You can't get rendered text on frames that have never been laid out, obviously....

Do the text change events need to happen sync?

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)

> Do the text change events need to happen sync?

No, but we coalesce them at this point, for example, if two text nodes are removed or inserted then we fire one text change events describing this change. Perhaps we could do smarter coalescence or do it after layout.

Is it the problem also to get rendered text on nsCSSFrameConstructor::RemoveContent(), no?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Yes, generally.  In that the state of the frame tree may be dirty at that point.

You could try to avoid doing it if the frame has never been reflowed at that point, maybe?

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #9)

> You could try to avoid doing it if the frame has never been reflowed at that
> point, maybe?

Does it concerns to ContentRangeInserted/ContentRemoved both? Frame that has never been reflowed means its text was never rendered?

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Does it concerns to ContentRangeInserted/ContentRemoved both? 

Yes, but you _know_ the text hasn't been reflowed after ContentRangeInserted.

> Frame that has never been reflowed means its text was never rendered?

The "never been reflowed" implies never rendered (though there are other things that can cause "never rendered"), yes.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #11)
> > Does it concerns to ContentRangeInserted/ContentRemoved both? 
> 
> Yes, but you _know_ the text hasn't been reflowed after ContentRangeInserted.

I'm not sure I follow. Could you give details how frame reflow is connected to ContentRangeInserted/ContentRemoved.

> > Frame that has never been reflowed means its text was never rendered?
> 
> The "never been reflowed" implies never rendered (though there are other things
> that can cause "never rendered"), yes.

Why does it happen? Usually the frame should be reflowed, right? Does it happen when frame was inserted and then removed "enough" quickly, so that we get notifications from ContentRangeInserted and ContentRemoved but frame wasn't reflowed.

Comment 13

[Boris Zbarsky [:bzbarsky]]

When ContentRangeInserted happens, we post an event to do the reflow.  This happens asynchronously.

ContentRemoved can happen before that event fires.  Or after.  Or after that reflow, but before some other reflow scheduled for the same frame (e.g. due to a style change or character data change).

> Usually the frame should be reflowed, right?

Eventually, yes.

> Does it happen when frame was inserted and then removed "enough" quickly

Yes.

Comment 14

[alexander :surkov (:asurkov)]

needs blocking+ because we may fire text change events with wrong information.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> You could try to avoid doing it if the frame has never been reflowed at that
> point, maybe?

How can I know that frame has never been reflowed? What API should I rely on?

Comment 16

[alexander :surkov (:asurkov)]

And how can I detect the frame is "dirty" (when I'm not allowed to get its rendered text, for example, when ContentRemoved is pended for it) that was inserted earlier (ContentRangeInserted triggered)?

Comment 17

[alexander :surkov (:asurkov)]

Boris, could you think of simple examples how I can test these cases?

Comment 18

[alexander :surkov (:asurkov)]

Btw, the problem is a bit wider than accessible text change event creation. We check RenderedText to decide whether should we create an accessible or not because we don't want to keep empty text nodes accessible. We do that when we are notified from ContentRangeInserted and at this point we're not allowed to get rendered text.

It sounds like all we can do is to collect ContentRangeInserted/ContentRemoved notifications and process them after layout. Boris, can you think of anything better or simpler?

Comment 19

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #18)

> It sounds like all we can do is to collect ContentRangeInserted/ContentRemoved
> notifications and process them after layout. Boris, can you think of anything
> better or simpler?

We might regress the user experience but I think this is safest.

Comment 20

[David Bolter [:davidb] (NeedInfo me for attention)]

So ContentRangeInserted should be processed after reflow is done, and for ContentRangeRemoved, we should check reflow status and try to get the information we need right away since I'm not sure queuing processing for this case would be useful. Does that make sense?

Comment 21

[(no longer active)]

Does it make sense to register an nsIReflowCallback and try to grab the frame's text (if it's still alive, of course) after the reflow has been finished completely?

Comment 22

[(no longer active)]

Also, note bug 613058 which I think is a dupe.

Comment 23

[alexander :surkov (:asurkov)]

Some chat with Boris to keep things together:

bz: the basic sequence of events is that something (JS or the parser) inserts some DOM nodes
[12:19am] bz: we set some flags on them and post an event to do the lazy frame construction
[12:19am] bz: once the frame construction happens, we post an event to do the layout of the new frames
[12:20am] bz: in general, arbitrary JS can run between the initial insertion and the frame construction and between the frame construction and layout
[12:20am] bz: with me so far?
[12:20am] surkov: yes
[12:20am] bz: ok
[12:21am] bz: now your questions from the bug..
[12:21am] bz: the NS_FRAME_FIRST_REFLOW set is set on a frame if and only if it has never been reflowed
[12:21am] bz: That was (1)
[12:22am] bz: (2) There is never a pending ContentRemoved; those are always processed eagerly
[12:23am] bz: (3) Testing this stuff just involves writing some JS to insert and remove DOM nodes...  You can trigger reflow by asking for layout properties (e.g. document.body.offsetWidth).  You can trigger frame construction but not reflow by asking for style properties that don't depend on layout (e.g. computed color)
[12:24am] surkov: ok
[12:24am] bz: (4) If your code relies on the correct "rendered text" value then it definitely needs to run after layout, I'd think, because we simply don't have information before then.  Alternately, your code could skip over not-yet-reflowed things, since a visual user wouldn't see them either (they're 0x0 in size)
[12:25am] bz: Which is easier sorta depends on how the rest of your architecture is set up; I'm not familiar with that.
[12:25am] bz: A complication is that reflows may not run to completion if they take too long, so even after a reflow some stuff may not be reflowed yet.  A new reflow event is posted in that situation.
[12:26am] surkov: it's possible to have not-yet-reflowed things for ContentRemoved only?
[12:26am] bz: what do you mean?
[12:26am] surkov: I mean about I coud skipt over not-yet-reflowed things
[12:26am] bz: When ContentRangeInserted/ContentAppended happen in the frame constructor, they produce some frames
[12:26am] bz: those frames have not been reflowed yet at that point
[12:26am] surkov: ok
[12:26am] bz: they will get reflowed some time later
[12:27am] bz: possibly a very long time later
[12:27am] bz: if there are a lot of them and the user is busy interacting with the page
[12:27am] surkov: I need to have rendered text before I create accessible for inserted text node (or more precisely I need to know if it has or will have some rendered text)
[12:28am] bz: yeah, you won't know that until after it lays out
[12:29am] bz: Worse yet... how do you define "rendered text"?
[12:30am] surkov: from API point of view nsIFrame::GetRenderedText, from visual if there's a text that may be visible on screen
[12:30am] surkov: so that user can iteract with it
[12:30am] surkov: or just see it
[12:30am] bz: <div>
[12:30am] bz: er...
[12:30am] bz: <div> <img src="x"> <img src="y"> </div>
[12:30am] bz: there are three text nodes here
[12:30am] bz: which if any of them have rendered text?
[12:30am] bz: just the one between the images?
[12:31am] surkov: any of them are really rendered (take some place on screen)?
[12:31am] bz: the one between the images sure does
[12:31am] bz: there's a space between the images
[12:32am] surkov: ok, so we need to have that one
[12:32am] bz: ok
[12:32am] bz: so now if I remove the second image
[12:32am] bz: from the DOM
[12:32am] bz: so there are still three text nodes in the DOM
[12:32am] bz: then the second textnode, which used to show up as a visible space, no longer shows up
[12:32am] surkov: another hint would be if I can traverse through these text nodes when caret navigation mode is on
[12:32am] bz: note that nothing about that text node has changed
[12:32am] bz: just what it's next to
[12:33am] surkov: it's tricky 
[12:33am] bz: seems to me like you really want text nodes to notify you when they gain or lose rendered text
[12:34am] bz: because detecting it otherwise is a huge pain
[12:34am] bz: (e.g. if that second image happened to be floating, the textnode would also not have any rendered text)
[12:34am] surkov: that should work and it should fix bug 613058
[12:34am] firebot2: Bug https://bugzilla.mozilla.org/show_bug.cgi?id=613058 nor, --, ---, nobody, NEW, nsIFrame::GetRenderedText may return wrong result on character change DOM mutations
[12:35am] bz: I'd talk to roc about what a good place to notify is
[12:35am] bz: he's more familiar with the text frame and textrun stuff than I am
[12:35am] surkov: ok, thank you
[12:35am] bz: no problem
[12:36am] bz: I wish layout were simpler.  
[12:36am] surkov: so the idea is to create accessible for text node iif it gets some rendered text?
[12:36am] surkov: I wish too
[12:36am] bz: that's what you want to do, right?
[12:36am] surkov: I think that should work
[12:36am] surkov: we really don't want to have accessible for text nodes that doesn't appear on screen
[12:37am] surkov: but if we will insert them into tree after layout then it should work I think
[12:37am] bz: yeah
[12:37am] bz: that's when a screen user would see them anyway
[12:38am] surkov: right

Comment 24

[alexander :surkov (:asurkov)]

Boris is it possible for example to get ContentRangeInserted, ContentRemoved and ContentRangeInserted for the same content before layout so that if I collect ContentRangeInserted calls to process them after layout then collected information may be out of date?

Comment 25

[Boris Zbarsky [:bzbarsky]]

> is it possible for example to get ContentRangeInserted, ContentRemoved
> and ContentRangeInserted for the same content before layout

Yes:

  <script>
    var div = document.createElement("div");
    document.body.appendChild(div);
    getComputedStyle(div, "").color;
    document.body.removeChild(div);
    document.body.appendChild(div);
  </script>

should do the trick.

Comment 26

[alexander :surkov (:asurkov)]

Attachment: idea

Collect all content inserted notifications to process them after layout, at that point store cached container accessible to recache its children after layout, then process all inserted children and fire show events for them. If they were removed before layout then we can't create an accessible for them and as result we don't fire show event for them. If they were inserted, removed and inserted again then we fire two show events which will be coalesced during event queue processing. This patch may result into excess reorder event on first container (in the case if content was reinserted into different part of subtree) but I think we could try to get protection on this.

The problem is other notifications (like DOM events) may occur before layout and we miss them because we didn't update accessible tree at this point and we don't have an accessible to fire a11y events for, for example, this happens for focus event. I think we should introduce more general notifications concept, i.e. collect all notifications and process them after layout so that we are guaranteed the tree is updated when we're about to fire events.

Does it sound good?

Comment 27

[David Bolter [:davidb] (NeedInfo me for attention)]

Yes. I'm not fond of us processing and firing any native platform events synchronously.

Comment 28

[David Bolter [:davidb] (NeedInfo me for attention)]

I think this is our top priority. I've added suspected dependent bug 615475 which might be the cause of topcrash bug 612098.

Comment 29

[alexander :surkov (:asurkov)]

Attachment: wip

Comment 30

[alexander :surkov (:asurkov)]

Boris, I have couple questions to you

1. Is there a way to know whether content insertions/removals or layout changes are pended. For example, when HTML input gets visible and then focused

input.setAttribute("style", "display: block;");
input.focus();

or

input.setAttribute("style", "visibility: visible");
input.focus();

When I get DOM focus event that is handled before content inserted or visibility style changed in these cases then can I know whether some layout processing is pendent?

I just want to avoid collecting of the DOM events notifications to process them when refresh observer triggers me if layout is in "good shape".

2. If I have registered refresh observers and when I flush layout FlushPendingNotifications(Flush_Layout) then these observers aren't triggered immediately after FlushPendingNotifications call? Is it doable?

Comment 31

[alexander :surkov (:asurkov)]

One more question, iirc you said the content insertions into input fields happens sync always (please correct me if I'm wrong) and the changes are handled in nsCSSFrameConstructor::AppendChild. I think it makes sense to process content insertions synchronously in a11y for this case and do not pend processing after layout. How can I know the content insertion was sync or async, ideally from ContentRangeInserted where a11y is notified about content insertion?

Comment 32

[alexander :surkov (:asurkov)]

(In reply to comment #31)
> One more question, iirc you said the content insertions into input fields
> happens sync always (please correct me if I'm wrong) and the changes are
> handled in nsCSSFrameConstructor::AppendChild. I think it makes sense to
> process content insertions synchronously in a11y for this case and do not pend
> processing after layout. How can I know the content insertion was sync or
> async, ideally from ContentRangeInserted where a11y is notified about content
> insertion?

This is kind of wrong. It looks like we're notified from nsCSSFrameConstructor::ContentAppended (not from ContentRangeInserted for sync insertions). The question is it possible to get ContentRangeInserted for input or its container, then ContentAppended for text nodes of input before layout?

Comment 33

[alexander :surkov (:asurkov)]

try server build - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-be301124a70c

Btw, it fixes bug 610317 for me. I suspect other related bugs should go away as well.

Comment 34

[alexander :surkov (:asurkov)]

Marco, I would like to get testing of bug 610317, bug 608724, bug 599814 and bug 610985 which might be related with this one.

Comment 35

[Marco Zehe (:MarcoZ)]

The win32 release build for this try-server build from comment #33 didn't show up, but for best results and comparison with regular nightly builds, I'd like to have that. Could you kick it off again?

Comment 36

[Marco Zehe (:MarcoZ)]

(In reply to comment #34)
> Marco, I would like to get testing of bug 610317, bug 608724, bug 599814 and
> bug 610985 which might be related with this one.

I am happy to report that all 4 of them are fixed with this try server build!

Comment 37

[David Bolter [:davidb] (NeedInfo me for attention)]

Awesome. What about bug 613058?

[WIP note: "pend" isn't a verb in english. Looking forward to reviewing when you're ready.]

Comment 38

[Marco Zehe (:MarcoZ)]

I just stumbled on some inconsistencies. While bug 610317 appeared to be fixed yesterday, this morning I was again able to reproduce it with this try-server build. Later, it disappeared again. Other bugs, however, seem to really be fixed. But this Wikipedia bug still sometimes seems to bite, but I haven't found what's causing it.

Comment 39

[alexander :surkov (:asurkov)]

Theoretically it can be possible until bug 606924 is fixed or in other words til AT can trigger accessible tree creation before layout.

Comment 40

[alexander :surkov (:asurkov)]

(In reply to comment #37)
> Awesome. What about bug 613058?

next

> [WIP note: "pend" isn't a verb in english. Looking forward to reviewing when
> you're ready.]

what are best verbs?

Comment 41

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #40)
> (In reply to comment #37)

> > [WIP note: "pend" isn't a verb in english. Looking forward to reviewing when
> > you're ready.]
> 
> what are best verbs?

Actually I found "pend" in a dictionary but it is obsolete and means "to hang".

It depends on the context. For "Pend tree update on content insertion until layout." probably "Delay tree update triggered by content insertion until layout is happy". For "NotificationController::PendEvent" probably "NotificationController::AppendEvent" is better.

Comment 42

[Marco Zehe (:MarcoZ)]

Additional report: Bug 610985 resurfaced even in the try-server build, but only intermittently. The instance where I saw it was in a secondary window as described in the original bug, but in the first window, there were TWO tabs open instead of only one.

And again, I still see bug 610317 more than I don't see it, don't know what's going on there, sometimes the form appears, sometimes it doesn't.

Bug 608724 and bug 599814, however, really seem to be fixed.

Comment 43

[alexander :surkov (:asurkov)]

Boris, when ContentRangeInserted/ContentAppended are called sync (if I get right when the following statement is true in this case: aAllowLazyConstruction == 1 && MaybeConstructLazily() == false) then rendered text of text nodes in subtree of inserted content may be not calculated yet?

Comment 44

[Boris Zbarsky [:bzbarsky]]

> Is there a way to know whether content insertions/removals or layout changes
> are pended.

There could be.  You can tell whether there are pending insertions/removals by examining flags on the documentElement, and the presshell knows whether there are pending reflows, and the frame constructor can ask the restyle trackers whether there are pending restyles.  So we could add some accessors to get at the relevant data.

> I just want to avoid collecting of the DOM events notifications to process
> them when refresh observer triggers me if layout is in "good shape".

Defined how?  What if I have |input:focus { width: 0; height: 0 }| in the in-page stylesheet?

> content insertions into input fields happens sync always

Correct.  

> How can I know the content insertion was sync or async, ideally from
> ContentRangeInserted 

If you're calling the a11y ContentRangeInserted from nsCSSFrameConstructor::ContentAppended, then it's a sync insert.

If you're calling it from nsCSSFrameConstructor::ContentRangeInserted and aAllowLazyConstruction is true, then it's a sync insert.

That said, an insert can happent sync for all sorts of reasons (see MaybeConstructLazily the cases when it returns false), not just because of editor stuff.  So I don't think you actually want the sync vs async distinction here...

> The question is it possible to get ContentRangeInserted for input
> or its container, then ContentAppended for text nodes of input before layout?

Yes, absolutely.  Along with ContentRemoved on the text nodes, etc.

> then rendered text of text nodes in subtree of inserted content may be not
> calculated yet?

Yep.  That's not computed until layout time, right?

Comment 45

[alexander :surkov (:asurkov)]

(In reply to comment #44)

> > then rendered text of text nodes in subtree of inserted content may be not
> > calculated yet?
> 
> Yep.  That's not computed until layout time, right?

Ok, I just wasn't sure about sync content insertions.

Comment 46

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 47

[alexander :surkov (:asurkov)]

(In reply to comment #44)
> > Is there a way to know whether content insertions/removals or layout changes
> > are pended.
> 
> There could be.  You can tell whether there are pending insertions/removals by
> examining flags on the documentElement, and the presshell knows whether there
> are pending reflows, and the frame constructor can ask the restyle trackers
> whether there are pending restyles.  So we could add some accessors to get at
> the relevant data.

In the patch I use IsLayoutFlushObserver() of refresh driver. Is this check enough or I should have additional checks?

> > I just want to avoid collecting of the DOM events notifications to process
> > them when refresh observer triggers me if layout is in "good shape".
> 
> Defined how?  What if I have |input:focus { width: 0; height: 0 }| in the
> in-page stylesheet?

A11y doesn't care about zero width and height (of course if content removed doesn't occur in this case). If it would be |input:focus { display: none } | or visibility: hidden then I would have the question where is the focus. But if DOM event is fired and sometime after I get content removed then it's ok to process DOM event before accessible tree update. Mostly I care about the case when new content is inserted, DOM events are fired for it and I handle these events before I handle content insertion - I shouldn't miss these events because accessible tree isn't updated.

Comment 48

[alexander :surkov (:asurkov)]

(In reply to comment #42)
> Additional report: Bug 610985 resurfaced even in the try-server build, but only
> intermittently. The instance where I saw it was in a secondary window as
> described in the original bug, but in the first window, there were TWO tabs
> open instead of only one.
> 
> And again, I still see bug 610317 more than I don't see it, don't know what's
> going on there, sometimes the form appears, sometimes it doesn't.

I filed bug 617300. It has all chances to be guilty of behavior you can see.

Comment 49

[alexander :surkov (:asurkov)]

try server build - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-b05b7eb02eb9/

Comment 50

[alexander :surkov (:asurkov)]

Attachment: patch2

linux bustage and crash

Comment 51

[alexander :surkov (:asurkov)]

(In reply to comment #50)
> Created attachment 496100 [details] [diff] [review]
> patch2
> 
> linux bustage and crash

try server build - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-92e531abd69a

Comment 52

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 496100 [details] [diff] [review]
patch2

nit: throughout, "pended" should be "appended"

>-  if (mEvents.Length() == 0) {
>-    nsCOMPtr<nsIPresShell> shell = mDocument->GetPresShell();
>-    if (!shell ||
>-        shell->RemoveRefreshObserver(this, Flush_Display)) {
>-      mObservingRefresh = PR_FALSE;
>-    }
>+  // Stop further notification processing if neither new notifications nor
>+  // events were pended during this processing.

Maybe: "Stop further processing if there are no newly appended insertions, notifications or events"

> nsresult
> nsDocAccessible::FireDelayedAccessibleEvent(AccEvent* aEvent)

>-  if (mEventQueue)
>-    mEventQueue->Push(aEvent);
>+  NS_ASSERTION(mNotificationController,
>+               "The document was shutdown when we're about to fire an event!");

I don't think we need this assertion.

>+  // Fire reorder event so the MSAA clients know the children have changed. Also
>+  // the event is used internally by MSAA part.

nit: MSAA layer.

>diff --git a/accessible/src/base/nsRootAccessible.h b/accessible/src/base/nsRootAccessible.h
>--- a/accessible/src/base/nsRootAccessible.h
>+++ b/accessible/src/base/nsRootAccessible.h

>-    nsresult AddEventListeners();
>-    nsresult RemoveEventListeners();
>+  /**
>+   * Add/remove DOM event listeners.
>+   */
>+  virtual nsresult AddEventListeners();
>+  virtual nsresult RemoveEventListeners();

Why did you make these virtual?

Comment 53

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 496100 [details] [diff] [review]
patch2

r=me. Hopefully bz can check the cycle collector macros which is still black magic to me.
(I think the templates are a bit overkill, but I don't mind)
You can disregard my comment about the virtual methods since that was just cleanup unrelated to this bug.

Comment 54

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 496100 [details] [diff] [review]
patch2

General comments:

1) TNotification holds a weak ref to its mInstance.  Why is that ok?  For example, what makes sure that the nsRootAccessible or nsCaretAccessible or whatever survives long enough for the notification to be processed?

2)  ContentInsertion holds a weak ref to mDocument.  Why is it ok?  The reasons should be documented.

>+++ b/accessible/src/base/NotificationController.cpp

>NotificationController::Shutdown()
>  if (mObservingState != eNoRefreshObserving) {
>    if (mPresShell->RemoveRefreshObserver(this, Flush_Display))

Why not:

  if (mObservingState != eNoRefreshObserving &&
      mPresShell->RemoveRefreshObserver(this, Flush_Display))

?

As far as that goes, I'd prefer eNotObservingRefresh for that value of the enum.

>+NotificationController::WillRefresh(mozilla::TimeStamp aTime)
> {
>+  // If the document accessible that notification collector was created for is
>+  // now shut down, don't process notifications anymore.

Do we need to clear our various notifications arrays in that case?  Or are we about to be deleted anyway?

>+  // Any generic notifications should be pended if we're processing content
>+  // insertions or generic notifications.

s/pended/deferred/ ?  Or "batched up" or "queued"?

>+  // notifications are pended during this processing then they will be processed
>+  // on next refresh.

s/pended/queued/ ?

> If notification processing pend new events then they are

s/pend/queues up/ or maybe "posts"?

> If events processing pend new events then new

s/pend/queues up/ or "posts".

>+  // Note: notification processing or event handling may shutdown the owning
>+  // document accessible.

"shut down". Two words.

>+  // If generic notification occurs after this point then we may be allowed to

"a generic notification"

>+  // Stop further notification processing if neither new notifications nor
>+  // events were pended during this processing.

s/pended/queued/

In your DEBUG_NOTIFICATIONS logging, why not just use ToUTF8String for the tag names too?

>+++ b/accessible/src/base/nsDocAccessible.h
>+   * Pend generic notification.

s/Pend/Queue/ ?

Please document the new method on nsIPresShell.  And you need to change the presshell iid, since you changed the vtable.  Which means that for gecko 2.0 at this point (well, anytime since October or so) you can't make this change to nsIPresShell.

I assume that you can't just inline that function on nsIPresShell and still have it compile in non-libxul builds?  If so, you need to add the function to a nre nsIPresShell_MOZILLA_2_0_BRANCH2 interface.

Not marking sr yet pending the nits above being resolved and answers to my questions about object lifetimes.

Comment 55

[alexander :surkov (:asurkov)]

Benjamin, it shouldn't be softbloker since it blocks hardblockers.

Comment 56

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 496100 [details] [diff] [review]
patch2

r- to make sure the presshell stuff is fixed.

Comment 57

[Boris Zbarsky [:bzbarsky]]

And when you post an updated version, could you post an interdiff as well?

Comment 58

[alexander :surkov (:asurkov)]

(In reply to comment #54)
> Comment on attachment 496100 [details] [diff] [review]
> patch2
> 
> General comments:
> 
> 1) TNotification holds a weak ref to its mInstance.  Why is that ok?  For
> example, what makes sure that the nsRootAccessible or nsCaretAccessible or
> whatever survives long enough for the notification to be processed?

nsCaretAccessible life cycle is controlled by nsRootAccessible (created when nsRootAcc created, destroyed when nsRootAcc is destroyed). nsRootAccessible lives longer than any its descendant document. We send notifications to notification controller of descendant document or nsRootAcc document here.

I can't keep in mind any other cases than nsCaretAcc and nsRootAcc for now. So I added comments.

> 2)  ContentInsertion holds a weak ref to mDocument.  Why is it ok?  The reasons
> should be documented.

ConentInsertion triggers the method on document that notification controller belong to. I've changed ScheduleContentInsertion to make this clearer.

> >  if (mObservingState != eNoRefreshObserving) {
> >    if (mPresShell->RemoveRefreshObserver(this, Flush_Display))
> 
> Why not:
> 
>   if (mObservingState != eNoRefreshObserving &&
>       mPresShell->RemoveRefreshObserver(this, Flush_Display))

no reason, perhaps artifact from clean up patch stage, fixed, thanks

> >+NotificationController::WillRefresh(mozilla::TimeStamp aTime)
> > {
> >+  // If the document accessible that notification collector was created for is
> >+  // now shut down, don't process notifications anymore.
> 
> Do we need to clear our various notifications arrays in that case?  Or are we
> about to be deleted anyway?

Yes, no mDocument means notification controller was shut down and arrays should be cleared.
> 
> In your DEBUG_NOTIFICATIONS logging, why not just use ToUTF8String for the tag
> names too?

thanks!

> I assume that you can't just inline that function on nsIPresShell and still
> have it compile in non-libxul builds?  If so, you need to add the function to a
> nre nsIPresShell_MOZILLA_2_0_BRANCH2 interface.

correct, fixed.

Comment 59

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 60

[alexander :surkov (:asurkov)]

(In reply to comment #57)
> And when you post an updated version, could you post an interdiff as well?

Does bugzilla interdiff not work for you? I tried to get interdiff between --mq revisions but that diff contains diff between diffs, it sounds that's not what you need. How can I do the interdiff?

Comment 61

[Boris Zbarsky [:bzbarsky]]

> Does bugzilla interdiff not work for you? 

Not reliably.  For one thing, it will silently leave out differences in many cases.  Which means I don't ever trust its output.

> How can I do the interdiff?

The simplest way to do an interdiff is to apply the old patch in your mq, then edit the file to look like it does after the new patch, then do an hg diff.

Comment 62

[alexander :surkov (:asurkov)]

Attachment: diff between patches

Boris, diff between patches is not very handy, right?

Comment 63

[alexander :surkov (:asurkov)]

(In reply to comment #61)

> > How can I do the interdiff?
> 
> The simplest way to do an interdiff is to apply the old patch in your mq, then
> edit the file to look like it does after the new patch, then do an hg diff.

Right, but is it still a simplest way when I did changes and qrefresh them?

Comment 64

[Boris Zbarsky [:bzbarsky]]

> Right, but is it still a simplest way when I did changes and qrefresh them?

Sure.  qpop your qrefreshed patch, qimport your old patch, qpush it, patch -R the old patch, apply the new patch, and hg diff.  I can try to do all that myself, but if there's any merging involved in any of that, I'll just give up on it.  And reviewing an interdiff would go a _lot_ faster than re-reviewing the whole thing while keeping my comments in mind.

The diff between patches is ... suboptimal.

Comment 65

[Marco Zehe (:MarcoZ)]

Alex, can we get a try-server build for this new patch?

Comment 66

[alexander :surkov (:asurkov)]

(In reply to comment #65)
> Alex, can we get a try-server build for this new patch?

sure, will be here - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-5e323ff3f6f5

Comment 67

[alexander :surkov (:asurkov)]

Attachment: interdiff

Comment 68

[Fernando Herrera]

With the last patch (patch3) applied to trunk I get this segfault:

#0  0x00f8e416 in __kernel_vsyscall ()
#1  0x0111e386 in nanosleep () from /lib/libc.so.6
#2  0x0111e1a4 in sleep () from /lib/libc.so.6
#3  0x00608b6d in ah_crap_handler (signum=11) at /home/fer/code/mozilla/toolkit/xre/nsSigHandlers.cpp:132
#4  0x0060b95c in nsProfileLock::FatalSignalHandler (signo=11, info=0xbfaff4ec, context=0xbfaff56c) at nsProfileLock.cpp:226
#5  <signal handler called>
#6  0x016ccdbe in NotificationController::IsUpdatePending (this=0xa8c100a0) at /home/fer/code/mozilla/accessible/src/base/NotificationController.cpp:158
#7  0x017168fe in NotificationController::HandleNotification<nsRootAccessible, nsIDOMEvent> (this=0xa8c100a0, aInstance=0xa9ed9150, aMethod=(void (nsRootAccessible::*)(nsRootAccessible *, nsIDOMEvent *)) 0x17133d2 <nsRootAccessible::ProcessDOMEvent(nsIDOMEvent*)>, aArg=0xaab9a280) at /home/fer/code/mozilla/accessible/src/base/NotificationController.h:159
#8  0x01715e0b in nsDocAccessible::HandleNotification<nsRootAccessible, nsIDOMEvent> (this=0xa9e65000, aInstance=0xa9ed9150, aMethod=(void (nsRootAccessible::*)(nsRootAccessible *, nsIDOMEvent *)) 0x17133d2 <nsRootAccessible::ProcessDOMEvent(nsIDOMEvent*)>, aArg=0xaab9a280) at /home/fer/code/mozilla/accessible/src/base/nsDocAccessible.h:205
#9  0x01713392 in nsRootAccessible::HandleEvent (this=0xa9ed9150, aDOMEvent=0xaab9a280) at /home/fer/code/mozilla/accessible/src/base/nsRootAccessible.cpp:480
#10 0x01713209 in nsRootAccessible::FireCurrentFocusEvent (this=0xa9ed9150) at /home/fer/code/mozilla/accessible/src/base/nsRootAccessible.cpp:433
#11 0x01744105 in nsAccessibleWrap::FirePlatformEvent (this=0xa9ed9150, aEvent=0xaabae1c0) at /home/fer/code/mozilla/accessible/src/atk/nsAccessibleWrap.cpp:1265
#12 0x01743231 in nsAccessibleWrap::HandleAccEvent (this=0xa9ed9150, aEvent=0xaabae1c0) at /home/fer/code/mozilla/accessible/src/atk/nsAccessibleWrap.cpp:1049
#13 0x01711671 in nsEventShell::FireEvent (aEvent=0xaabae1c0) at /home/fer/code/mozilla/accessible/src/base/nsEventShell.cpp:63
#14 0x0171174c in nsEventShell::FireEvent (aEventType=69, aAccessible=0xa9ed9150, aIsFromUserInput=eAutoDetect) at /home/fer/code/mozilla/accessible/src/base/nsEventShell.cpp:77
#15 0x016f7498 in nsAccessibilityService::FireAccessibleEvent (this=0xaaba5280, aEvent=69, aTarget=0xa9ed9150) at /home/fer/code/mozilla/accessible/src/base/nsAccessibilityService.cpp:173
#16 0x013eadba in nsWindow::DispatchEventToRootAccessible (this=0xb3a80f00, aEventType=69) at /home/fer/code/mozilla/widget/src/gtk2/nsWindow.cpp:6423
#17 0x013eade7 in nsWindow::DispatchActivateEventAccessible (this=0xb3a80f00) at /home/fer/code/mozilla/widget/src/gtk2/nsWindow.cpp:6430
#18 0x013dcb10 in nsWindow::DispatchActivateEvent (this=0xb3a80f00) at /home/fer/code/mozilla/widget/src/gtk2/nsWindow.cpp:538
#19 0x013e2351 in nsWindow::OnContainerFocusInEvent (this=0xb3a80f00, aWidget=0xb3a27dd0 [MozContainer], aEvent=0xaac05380) at /home/fer/code/mozilla/widget/src/gtk2/nsWindow.cpp:2883


where:

#6  0x016ccdbe in NotificationController::IsUpdatePending (this=0xa8c100a0) at /home/fer/code/mozilla/accessible/src/base/NotificationController.cpp:158
158	  return presShell->IsLayoutFlushObserver() ||
159	    mObservingState == eRefreshProcessingForUpdate ||
160	    mContentInsertions.Length() != 0 || mNotifications.Length() != 0;

Comment 69

[Fernando Herrera]

Sorry, ignore my last comment, bad build.

Comment 70

[Marco Zehe (:MarcoZ)]

Just a quick note that I've been running with this try-server build for the last 2 days, and it really looks good usability-wise. Less of a problem with document loads not coming through, stable Wikipedia form appearance and other fixes.

Comment 71

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #66)
> (In reply to comment #65)
> > Alex, can we get a try-server build for this new patch?
> 
> sure, will be here -
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-5e323ff3f6f5

No 64 bit build?

Drive-by note: I tested a local non-debug build of trunk today as well as a nightly. I could recreate this bug easily in the nightly but not in my local build, which indicates a mozconfig difference (likely optimization and jemalloc?) which in turn points to timing.

Comment 72

[alexander :surkov (:asurkov)]

> Status Whiteboard|[softblocker][dupe 613058?] |[hardblocker][dupe 613058?]

bug 613058 is not dupe

Comment 73

[alexander :surkov (:asurkov)]

Boris, ping?

Comment 74

[Boris Zbarsky [:bzbarsky]]

> Yes, no mDocument means notification controller was shut down and arrays should
> be cleared.

Please add an assert for that.

Comment 75

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 502316 [details] [diff] [review]
patch3

>+ *        longer than the document accessible owning notification controller

"owning the notification controller"

>+   * Process the generic notification synchronously if layout is not awaiting
>+   * the refresh and no pending or proceeding notifications, otherwise queue
>+   * it up to process asynchronously.

How about:

  Process the generic notification synchronously if there are no pending layout
  changes and no notifications are pending or being processed right now. 
  Otherwise, queue it up to process asynchronously.

>+ * @note  The caller must guarantee that the given instance still exist when
>+   *        notification is processed.

"when the notification"

>+  // The caret accessible life cycle is controlled by root accessible what
>+  // exists longer than any its descendant document so that we are guaranteed
>+  // notification is processed before caret accessible is destroyed.

Perhaps:

  The caret accessible has the same lifetime as the root accessible, and this
  outlives all its descendant document accessibles, so that we are guaranteed
  that the notification is processed before the caret accessible is destroyed.

>+    // Root accessible exists longer than any its descendant document so that

"any of its descendant documents"

sr=me with those nits and the assert nit in the previous comment addressed.  Sorry for this last bit of lag, and thanks for doing this!

Comment 76

[alexander :surkov (:asurkov)]

(In reply to comment #75)

> sr=me with those nits and the assert nit in the previous comment addressed. 
> Sorry for this last bit of lag, and thanks for doing this!

fixed locally, thank you for review, the code looks much better now.

Comment 77

[alexander :surkov (:asurkov)]

landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/eb105fe0e41c

Comment 78

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #77)
> landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/eb105fe0e41c

Excellent. Let's keep an eye out to see if bug 612098 persists.