update accessible tree on content insertion after layout

RESOLVED FIXED in mozilla2.0b10

Status

()

defect
P1
major
RESOLVED FIXED
10 years ago
5 years ago

People

(Reporter: mats, Assigned: surkov)

Tracking

({assertion, regression})

unspecified
mozilla2.0b10
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [hardblocker])

Attachments

(3 attachments, 5 obsolete attachments)

Reporter

Description

10 years ago
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)
Reporter

Comment 1

10 years ago
Posted file stack
Yeah we need to call FlushPendingNotifications I think. I'll try to get to this one. What is the severity?
Assignee

Comment 3

10 years ago
I'm not sure it affect on screen readers users but it brings a pain to debug on windows:)
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?
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.
Assignee

Comment 6

9 years ago
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?
You can't get rendered text on frames that have never been laid out, obviously....

Do the text change events need to happen sync?
Assignee

Comment 8

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

Updated

9 years ago
Blocks: 571613
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?
Assignee

Comment 10

9 years ago
(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?
> 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.
Assignee

Comment 12

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

Comment 14

9 years ago
needs blocking+ because we may fire text change events with wrong information.
blocking2.0: --- → ?
Assignee

Comment 15

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

Comment 16

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

Comment 17

9 years ago
Boris, could you think of simple examples how I can test these cases?
Assignee

Comment 18

9 years ago
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?
(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.
blocking2.0: ? → final+
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

9 years ago
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

9 years ago
Also, note bug 613058 which I think is a dupe.
Assignee

Comment 23

9 years ago
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
blocking2.0: final+ → betaN+
Assignee

Comment 24

9 years ago
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?
> 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.
Assignee

Comment 26

9 years ago
Posted patch idea (obsolete) — Splinter Review
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?
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Yes. I'm not fond of us processing and firing any native platform events synchronously.
Severity: normal → major
Priority: -- → P2
I think this is our top priority. I've added suspected dependent bug 615475 which might be the cause of topcrash bug 612098.
Blocks: 615475
Priority: P2 → P1
Assignee

Comment 29

9 years ago
Posted patch wip (obsolete) — Splinter Review
Attachment #492666 - Attachment is obsolete: true
Assignee

Comment 30

9 years ago
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?
Assignee

Comment 31

9 years ago
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?
Assignee

Comment 32

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

Comment 33

9 years ago
try server build - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/surkov.alexander@gmail.com-be301124a70c

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

Updated

9 years ago
Blocks: 610317
Assignee

Comment 34

9 years ago
Marco, I would like to get testing of bug 610317, bug 608724, bug 599814 and bug 610985 which might be related with this one.
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?
(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!
Awesome. What about bug 613058?

[WIP note: "pend" isn't a verb in english. Looking forward to reviewing when you're ready.]
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.
Assignee

Comment 39

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

Comment 40

9 years ago
(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?
(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.
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.
Assignee

Comment 43

9 years ago
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?
Assignee

Updated

9 years ago
Summary: ASSERTION: Can only call this on frames that have been reflowed: '!(GetStateBits() & NS_FRAME_FIRST_REFLOW)' → update accessible tree on content insertion after layout
> 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?
Assignee

Comment 45

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

Comment 46

9 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #494649 - Attachment is obsolete: true
Attachment #495793 - Flags: superreview?(bzbarsky)
Attachment #495793 - Flags: review?(bolterbugz)
Assignee

Comment 47

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

Updated

9 years ago
Blocks: 617300
Assignee

Comment 48

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

Updated

9 years ago
Blocks: 606924
Assignee

Comment 50

9 years ago
Posted patch patch2 (obsolete) — Splinter Review
linux bustage and crash
Attachment #495793 - Attachment is obsolete: true
Attachment #496100 - Flags: superreview?(bzbarsky)
Attachment #496100 - Flags: review?(bolterbugz)
Attachment #495793 - Flags: superreview?(bzbarsky)
Attachment #495793 - Flags: review?(bolterbugz)
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 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.
Attachment #496100 - Flags: review?(bolterbugz) → review+

Updated

9 years ago
Keywords: regression
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.

Updated

9 years ago
Whiteboard: [softblocker]
Assignee

Comment 55

9 years ago
Benjamin, it shouldn't be softbloker since it blocks hardblockers.
Comment on attachment 496100 [details] [diff] [review]
patch2

r- to make sure the presshell stuff is fixed.
Attachment #496100 - Flags: superreview?(bzbarsky) → superreview-
And when you post an updated version, could you post an interdiff as well?
Assignee

Comment 58

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

Comment 59

9 years ago
Posted patch patch3Splinter Review
Attachment #496100 - Attachment is obsolete: true
Attachment #502316 - Flags: superreview?(bzbarsky)
Assignee

Comment 60

9 years ago
(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?
> 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.
Assignee

Comment 62

9 years ago
Posted patch diff between patches (obsolete) — Splinter Review
Boris, diff between patches is not very handy, right?
Assignee

Comment 63

9 years ago
(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?
> 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.
Alex, can we get a try-server build for this new patch?
Assignee

Comment 66

9 years ago
(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/surkov.alexander@gmail.com-5e323ff3f6f5
Assignee

Comment 67

9 years ago
Posted patch interdiffSplinter Review
Attachment #502421 - Attachment is obsolete: true
Assignee

Updated

9 years ago
Blocks: 623445

Comment 68

9 years ago
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

9 years ago
Sorry, ignore my last comment, bad build.
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.
Whiteboard: [softblocker] → [softblocker][dupe 613058?]
(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/surkov.alexander@gmail.com-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.
Assignee

Updated

9 years ago
Blocks: 625653
Whiteboard: [softblocker][dupe 613058?] → [hardblocker][dupe 613058?]
Assignee

Comment 72

9 years ago
> Status Whiteboard|[softblocker][dupe 613058?] |[hardblocker][dupe 613058?]

bug 613058 is not dupe
Whiteboard: [hardblocker][dupe 613058?] → [hardblocker]
Assignee

Comment 73

9 years ago
Boris, ping?
> Yes, no mDocument means notification controller was shut down and arrays should
> be cleared.

Please add an assert for that.
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!
Attachment #502316 - Flags: superreview?(bzbarsky) → superreview+
Assignee

Comment 76

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

Comment 77

9 years ago
landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/eb105fe0e41c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b10
(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.

Updated

5 years ago
Depends on: 1059020
You need to log in before you can comment on or make changes to this bug.