869806 - fix asserts in actions/test_link.html

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

What happens is more or less this
js clicks a link
a new page comes up
we fire a document loaded event first to js
the js that is called to handle the event closes the window shutting down the doc accessible that is the target of the the document load complete event.
we return from the js event handler and call AccessibleWrap::FirePlatformEvent() which is unhappy because aEvent->GetAccessible() is now defunct.

I think the reasonable thing to do here is just change the test so we close the window after returning from the event handler.

Comment 1

[Trevor Saunders (:tbsaunde)]

Alex have thoughts / ideas how to implement that nicely?

Comment 2

[alexander :surkov (:asurkov)]

which assertion? It doesn't seem we should assert when node is defunct.

Comment 3

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #2)
> which assertion? It doesn't seem we should assert when node is defunct.

AccessibleWrap.cpp:970  Why shouldn't we assert?

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> (In reply to alexander :surkov from comment #2)
> > which assertion? It doesn't seem we should assert when node is defunct.
> 
> AccessibleWrap.cpp:970  Why shouldn't we assert?

well, I don't think the assertion case is possible, but its introduction looks reasonable in bug 383434. Then we can change assertion taking into account the accessible may be defunct.

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #4)
> (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > (In reply to alexander :surkov from comment #2)
> > > which assertion? It doesn't seem we should assert when node is defunct.
> > 
> > AccessibleWrap.cpp:970  Why shouldn't we assert?
> 
> well, I don't think the assertion case is possible, but its introduction
> looks reasonable in bug 383434. Then we can change assertion taking into
> account the accessible may be defunct.

when is it reasonable for the target of event to be defunct?  don't we specifically fire hide before it becomes defunct?

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)

> > well, I don't think the assertion case is possible, but its introduction
> > looks reasonable in bug 383434. Then we can change assertion taking into
> > account the accessible may be defunct.
> 
> when is it reasonable for the target of event to be defunct?  

if I understood right this is an example you described in the bug. 

> don't we
> specifically fire hide before it becomes defunct?

we do but if js makes sync changes on the event then it may be defunct when it's ready to be fired to the platform. Do I miss something?

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> 
> > > well, I don't think the assertion case is possible, but its introduction
> > > looks reasonable in bug 383434. Then we can change assertion taking into
> > > account the accessible may be defunct.
> > 
> > when is it reasonable for the target of event to be defunct?  
> 
> if I understood right this is an example you described in the bug. 

I'm not convinced I think this example is reasonable though

> > don't we
> > specifically fire hide before it becomes defunct?
> 
> we do but if js makes sync changes on the event then it may be defunct when
> it's ready to be fired to the platform. Do I miss something?

no, but only trusted code can listen for those events, so maybe we should say that code can't do things that will cause other accessible events to be dispatched first.  As it is you'll get the document loaded notification after focus / state changes / whatever which seems kind of bad.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #7)
 
> no, but only trusted code can listen for those events, so maybe we should
> say that code can't do things that will cause other accessible events to be
> dispatched first.

assumption of this sort may lead to hard-to-debug problems and makes a problem for a11y web api.

>  As it is you'll get the document loaded notification
> after focus / state changes / whatever which seems kind of bad.

sometimes events order makes difference, but document loaded events and focus events order shouldn't do.

actually I'm confused why you talk about events ordering at all, at least I don't see a connection with bug description. Also I'm not sure why the example from bug description is not reasonable.

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #8)
> (In reply to Trevor Saunders (:tbsaunde) from comment #7)
>  
> > no, but only trusted code can listen for those events, so maybe we should
> > say that code can't do things that will cause other accessible events to be
> > dispatched first.
> 
> assumption of this sort may lead to hard-to-debug problems and makes a

not sure what you mean, I'm not suggesting that we rely on requirement for our own code, but that we should document that you shouldn't cause events to be dispatched in the event handler and shouldn't write js that does that ourselves.

> problem for a11y web api.

not sure what api you think of.

> >  As it is you'll get the document loaded notification
> > after focus / state changes / whatever which seems kind of bad.
> 
> sometimes events order makes difference, but document loaded events and
> focus events order shouldn't do.

sure, but focus is just an example, I suspect you get hide events for document before load complete too which while maybe ok is pretty weird.

> actually I'm confused why you talk about events ordering at all, at least I
> don't see a connection with bug description. Also I'm not sure why the
> example from bug description is not reasonable.

I'm not convinced that what's happening is reasonable, and part of that is that it does crazy things to event order.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment
> > assumption of this sort may lead to hard-to-debug problems and makes a
> 
> not sure what you mean, I'm not suggesting that we rely on requirement for
> our own code, but that we should document that you shouldn't cause events to
> be dispatched in the event handler and shouldn't write js that does that
> ourselves.

I was grown assuming that JS lets you do not to think about under-the-hood things and it doesn't practice the idea that you shouldn't go there because snow falls from the roof :) seriously, making js author to care what he can do because of events firing is too tough requirement for js code.

> > problem for a11y web api.
> 
> not sure what api you think of.

like webkit web api, just an api that allows your web page to access the info that AT operates with 

> > >  As it is you'll get the document loaded notification
> > > after focus / state changes / whatever which seems kind of bad.
> > 
> > sometimes events order makes difference, but document loaded events and
> > focus events order shouldn't do.
> 
> sure, but focus is just an example, I suspect you get hide events for
> document before load complete too which while maybe ok is pretty weird.

I'd argue that we shouldn't fire document load event in this case

> > actually I'm confused why you talk about events ordering at all, at least I
> > don't see a connection with bug description. Also I'm not sure why the
> > example from bug description is not reasonable.
> 
> I'm not convinced that what's happening is reasonable, and part of that is
> that it does crazy things to event order.

I;d say we should handle crazy things on our end, letting JS doing a wild things.

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #10)
> (In reply to Trevor Saunders (:tbsaunde) from comment
> > > assumption of this sort may lead to hard-to-debug problems and makes a
> > 
> > not sure what you mean, I'm not suggesting that we rely on requirement for
> > our own code, but that we should document that you shouldn't cause events to
> > be dispatched in the event handler and shouldn't write js that does that
> > ourselves.
> 
> I was grown assuming that JS lets you do not to think about under-the-hood
> things and it doesn't practice the idea that you shouldn't go there because
> snow falls from the roof :) seriously, making js author to care what he can
> do because of events firing is too tough requirement for js code.

that's not an enirely unreasonable view, but allowing seems tricky.

> > > problem for a11y web api.
> > 
> > not sure what api you think of.
> 
> like webkit web api, just an api that allows your web page to access the
> info that AT operates with 

not sure what webkit thing you talk about, but ok


> > > >  As it is you'll get the document loaded notification
> > > > after focus / state changes / whatever which seems kind of bad.
> > > 
> > > sometimes events order makes difference, but document loaded events and
> > > focus events order shouldn't do.
> > 
> > sure, but focus is just an example, I suspect you get hide events for
> > document before load complete too which while maybe ok is pretty weird.
> 
> I'd argue that we shouldn't fire document load event in this case

I'd prefer that we fired document load when it would have fired had js not done funny things, but that seems acceptable.

> > > actually I'm confused why you talk about events ordering at all, at least I
> > > don't see a connection with bug description. Also I'm not sure why the
> > > example from bug description is not reasonable.
> > 
> > I'm not convinced that what's happening is reasonable, and part of that is
> > that it does crazy things to event order.
> 
> I;d say we should handle crazy things on our end, letting JS doing a wild
> things.

 how? would you be ok with firing a runnable at the main thread that dispatched xpcom events so they couldn't effect platform ones and platform ones couldn't effect them?

Comment 12

[alexander :surkov (:asurkov)]

having several events listeners that can do actions is ok in general. If one listener shutdowns the object the event was dispatched to then I would stop this event dispatching to other listeners.

Comment 13

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #12)
> having several events listeners that can do actions is ok in general. If one
> listener shutdowns the object the event was dispatched to then I would stop
> this event dispatching to other listeners.

not dispatching seems better than dispatching far too late, but I really don't like it very much that platform things will never know about things because js did something really odd.

I'm also not entirely sure that the rest of NotificationController::Willrefresh() will handle this case sanely.  So I'd sort of prefer the runnable thing even   without the event dispatch issue.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #13)
> (In reply to alexander :surkov from comment #12)
> > having several events listeners that can do actions is ok in general. If one
> > listener shutdowns the object the event was dispatched to then I would stop
> > this event dispatching to other listeners.
> 
> not dispatching seems better than dispatching far too late, but I really
> don't like it very much that platform things will never know about things
> because js did something really odd.

JS just closed window, it's "certified" operation. JS constantly changes the web page and AT may not know about some things, for example, if you do window.open(); window.close() then I think AT won't know about this.

> I'm also not entirely sure that the rest of
> NotificationController::Willrefresh() will handle this case sanely.  So I'd
> sort of prefer the runnable thing even   without the event dispatch issue.

what's wrong with NotificationController::Willrefresh() and how does runnable help?

Comment 15

[Trevor Saunders (:tbsaunde)]

> > I'm also not entirely sure that the rest of
> > NotificationController::Willrefresh() will handle this case sanely.  So I'd
> > sort of prefer the runnable thing even   without the event dispatch issue.
> 
> what's wrong with NotificationController::Willrefresh() and how does

its called on an object when there's a call into it for the same object already on the stack?  I'm not absolutely sure anything is broken, but its funny situation that it might well not handle well.  Its more or less  spinning a nested event loop (the js could infact spin the event loop too).

> runnable help?

there'd be no code on the stack holding state it expects to stay valid?  And it would be clearer that the code firing the events shouldn't hold state, but it won't have a reason to anyway.

Comment 16

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #15)
> > > I'm also not entirely sure that the rest of
> > > NotificationController::Willrefresh() will handle this case sanely.  So I'd
> > > sort of prefer the runnable thing even   without the event dispatch issue.
> > 
> > what's wrong with NotificationController::Willrefresh() and how does
> 
> its called on an object when there's a call into it for the same object
> already on the stack?

WillRefresh() is running under assumption that linked DocAccessible can go away. Is it you talk about or you mean something else?

Comment 17

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #16)
> (In reply to Trevor Saunders (:tbsaunde) from comment #15)
> > > > I'm also not entirely sure that the rest of
> > > > NotificationController::Willrefresh() will handle this case sanely.  So I'd
> > > > sort of prefer the runnable thing even   without the event dispatch issue.
> > > 
> > > what's wrong with NotificationController::Willrefresh() and how does
> > 
> > its called on an object when there's a call into it for the same object
> > already on the stack?
> 
> WillRefresh() is running under assumption that linked DocAccessible can go
> away. Is it you talk about or you mean something else?

I guess firing events is close to the last thing it does, but I think the concern is more general than just unlinking document.  Consider that document can stay conected, but still have been changed in arbitrary ways.

Comment 18

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #17)

> I guess firing events is close to the last thing it does, but I think the
> concern is more general than just unlinking document.  Consider that
> document can stay conected, but still have been changed in arbitrary ways.

the worst thing can happen that delivered events are out of date but that will happen for async listeners in either case

Comment 19

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #18)
> (In reply to Trevor Saunders (:tbsaunde) from comment #17)
> 
> > I guess firing events is close to the last thing it does, but I think the
> > concern is more general than just unlinking document.  Consider that
> > document can stay conected, but still have been changed in arbitrary ways.
> 
> the worst thing can happen that delivered events are out of date but that
> will happen for async listeners in either case

not sure what you refer to

Comment 20

[alexander :surkov (:asurkov)]

I don't really see any problems that document changes its state or even gets shutdown during events flushing.

Comment 21

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #20)
> I don't really see any problems that document changes its state or even gets
> shutdown during events flushing.

are you sure we'll never try to remove ourselves as a flush observer twice? Or nothing else funny can happen like us remove ourselves when we didn't actually want to?

is there a reason you don't want to do runnable thing?

Comment 22

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #21)
> (In reply to alexander :surkov from comment #20)
> > I don't really see any problems that document changes its state or even gets
> > shutdown during events flushing.
> 
> are you sure we'll never try to remove ourselves as a flush observer twice?

not absolutely, mObservingState changing logic is not plain but in either case it shouldn't be bad if we remove ourselves twice.

> Or nothing else funny can happen like us remove ourselves when we didn't
> actually want to?

Well, I agree we should be careful about mObservingState logic. I should notice it worked for a while and no known problems.

> is there a reason you don't want to do runnable thing?

it slows down things and adds extra level of complexity.

Comment 23

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #22)
> (In reply to Trevor Saunders (:tbsaunde) from comment #21)
> > (In reply to alexander :surkov from comment #20)
> > > I don't really see any problems that document changes its state or even gets
> > > shutdown during events flushing.
> > 
> > are you sure we'll never try to remove ourselves as a flush observer twice?
> 
> not absolutely, mObservingState changing logic is not plain but in either
> case it shouldn't be bad if we remove ourselves twice.
> 
> > Or nothing else funny can happen like us remove ourselves when we didn't
> > actually want to?
> 
> Well, I agree we should be careful about mObservingState logic. I should
> notice it worked for a while and no known problems.

yeah, but I doubt many things listen for events in js so I'm not sure we would have heard about problems even if they exist.  And if we did debugging this doesn't seem simple.

> > is there a reason you don't want to do runnable thing?
> 
> it slows down things and adds extra level of complexity.

what does it slow down that we care about the performance of?  I doubt it'll make things much more complex, but sure.

Comment 24

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #23)

> yeah, but I doubt many things listen for events in js

that can be quite simple: a collisions between a web page and content add-on.

> > > is there a reason you don't want to do runnable thing?
> > 
> > it slows down things and adds extra level of complexity.
> 
> what does it slow down that we care about the performance of?  I doubt it'll
> make things much more complex, but sure.

It add an extra layer and I'm not sure the code logic really wins. I'd rather added assertions.

Comment 25

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #24)
> (In reply to Trevor Saunders (:tbsaunde) from comment #23)
> 
> > yeah, but I doubt many things listen for events in js
> 
> that can be quite simple: a collisions between a web page and content add-on.

yeah, but I suspect there is few if any addons that listen for accessible events in the first place.

> > > > is there a reason you don't want to do runnable thing?
> > > 
> > > it slows down things and adds extra level of complexity.
> > 
> > what does it slow down that we care about the performance of?  I doubt it'll
> > make things much more complex, but sure.
> 
> It add an extra layer and I'm not sure the code logic really wins. I'd
> rather added assertions.

what exactly do you want to assert?

Comment 26

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #25)

> > > yeah, but I doubt many things listen for events in js
> > 
> > that can be quite simple: a collisions between a web page and content add-on.
> 
> yeah, but I suspect there is few if any addons that listen for accessible
> events in the first place.

I meant DOM events.

> what exactly do you want to assert?

what you mentioned above: we add a listener when we have a listener already, we remove a listener when we shouldn't remove.

Comment 27

[Trevor Saunders (:tbsaunde)]

Attachment: bug 869806 - fix assertion about event type in accessibleWrap.cpp

Comment 28

[alexander :surkov (:asurkov)]

Comment on attachment 753917 [details] [diff] [review]
bug 869806 - fix assertion about event type in accessibleWrap.cpp

Review of attachment 753917 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with fixed/answered

::: accessible/src/atk/AccessibleWrap.cpp
@@ +953,5 @@
>  
>      Accessible* accessible = aEvent->GetAccessible();
>      NS_ENSURE_TRUE(accessible, NS_ERROR_FAILURE);
> +    if (accessible->IsDefunct())
> +      return NS_OK;

I'd be good to comment here how it can happen

nit: wrong indentation (4 spaces vs 2 spaces indent)

::: accessible/src/base/NotificationController.cpp
@@ +269,5 @@
>    mDocument->ProcessInvalidationList();
>  
>    // If a generic notification occurs after this point then we may be allowed to
>    // process it synchronously.
> +  mObservingState = eRefreshProcessing;

pls, extend the comment saying that we don't want to reenter if event listener triggers the layout. Btw, are you sure that WillRefresh can be called sync? Bz said something about this but I don't recall it.

@@ +277,2 @@
>    if (!mDocument)
>      return;

you don't get to the previous state when we return early since I agree it doesn't make sense. But then it'd be good to move mObservingState = eRefreshObserving after !mDocument check to stay consistent.

Comment 29

[Trevor Saunders (:tbsaunde)]

> ::: accessible/src/base/NotificationController.cpp
> @@ +269,5 @@
> >    mDocument->ProcessInvalidationList();
> >  
> >    // If a generic notification occurs after this point then we may be allowed to
> >    // process it synchronously.
> > +  mObservingState = eRefreshProcessing;
> 
> pls, extend the comment saying that we don't want to reenter if event
> listener triggers the layout. Btw, are you sure that WillRefresh can be
> called sync? Bz said something about this but I don't recall it.

he just said to me it can reenter if it called script which is the case here.

> @@ +277,2 @@
> >    if (!mDocument)
> >      return;
> 
> you don't get to the previous state when we return early since I agree it
> doesn't make sense. But then it'd be good to move mObservingState =
> eRefreshObserving after !mDocument check to stay consistent.

huh? I believe this  never changes what mObservingState is when we leave the function relative to what it was before the patch.

Comment 30

[alexander :surkov (:asurkov)]

I meant that you do

mObservingState = eRefreshObserving;

before 

if (!mDocument)
  return;

you don't care about state if there's no mDocument, right? So you can change the state after this check

Comment 31

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #30)
> I meant that you do
> 
> mObservingState = eRefreshObserving;
> 
> before 
> 
> if (!mDocument)
>   return;
> 
> you don't care about state if there's no mDocument, right? So you can change
> the state after this check

but what's the advantageof that? it seems simpler to leave the behavior in that case as  is.

Comment 32

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #31)

> but what's the advantageof that? it seems simpler to leave the behavior in
> that case as  is.

just stay consistent, you don't change the state before mDocument check above

Comment 33

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #32)
> (In reply to Trevor Saunders (:tbsaunde) from comment #31)
> 
> > but what's the advantageof that? it seems simpler to leave the behavior in
> > that case as  is.
> 
> just stay consistent, you don't change the state before mDocument check above

that sort of consistancy for its own sake seems pretty silly to me, not that I'm even completely sure what you're trying to be consistant with.  It also seems nice that its clear that mObserveringState will be a state that allows reentry or is not observing when you leave the function which isn't true if you change the order.  Maybe that isn't important today, but why bother change it?

Comment 34

[alexander :surkov (:asurkov)]

it's not really important. Just if you change the state before mDocument check then I think of a reason why it matters.

Comment 35

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #34)
> it's not really important. Just if you change the state before mDocument
> check then I think of a reason why it matters.

I just tried to not changes things in ways I didn't have too.

Comment 36

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8b5a5697a30

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d8b5a5697a30