Last Comment Bug 852044 - don't fire events from DocAccessible::ProcessLoad() sync
: don't fire events from DocAccessible::ProcessLoad() sync
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla22
Assigned To: Trevor Saunders (:tbsaunde)
:
: alexander :surkov
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-17 22:57 PDT by Trevor Saunders (:tbsaunde)
Modified: 2013-03-22 07:01 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (2.11 KB, patch)
2013-03-17 23:01 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Splinter Review

Description Trevor Saunders (:tbsaunde) 2013-03-17 22:57:21 PDT
this causes asserts because firing the event causes us to run script which tries to close a window which of course causes a reflow while NotificationController is processing updates.  This passes all tests, but I'm not sure hwo many asserts it fixes on !linux yet.
Comment 1 Trevor Saunders (:tbsaunde) 2013-03-17 23:01:19 PDT
Created attachment 726005 [details] [diff] [review]
patch

this is also good because it means these events participant in coalescence logic.
Comment 2 alexander :surkov 2013-03-19 19:57:17 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #1)

> this is also good because it means these events participant in coalescence
> logic.

only if busy state change event (if somebody plays with aria-busy on loading document)

[note to myself] so we just fire document load/state busy events after all events in the queue. Taking into account we could fire some events on the document content before this point then it shouldn't be bad.

Trev, do you think to do the same for generic notification processing?
Comment 3 alexander :surkov 2013-03-19 19:57:59 PDT
Comment on attachment 726005 [details] [diff] [review]
patch

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

r=me
Comment 4 Trevor Saunders (:tbsaunde) 2013-03-19 20:09:46 PDT
> Trev, do you think to do the same for generic notification processing?

yes I think so
Comment 5 Trevor Saunders (:tbsaunde) 2013-03-19 23:06:43 PDT
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/221a7a558c63
Comment 6 Richard Newman [:rnewman] 2013-03-20 00:57:19 PDT
ewong's about to back this out for bustage, so I won't set target milestone, but I'll set assignee while I'm here. :D

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/events/test_scroll.xul | Test timed out

https://tbpl.mozilla.org/php/getParsedLog.php?id=20867947&tree=Mozilla-Inbound
Comment 7 Trevor Saunders (:tbsaunde) 2013-03-21 12:58:39 PDT
Jamie this will make it somewhat more likely that scrolling start will fire before document load (which could already happen in some cases)  Could that be a problem for you?
Comment 8 James Teh [:Jamie] 2013-03-21 20:16:52 PDT
It's not a problem if scrolling start fires before document load. However, it mustn't fire before initial focus.
Comment 9 Trevor Saunders (:tbsaunde) 2013-03-21 21:31:22 PDT
(In reply to James Teh [:Jamie] from comment #8)
> It's not a problem if scrolling start fires before document load. However,
> it mustn't fire before initial focus.

we preserve ordering of scroll start event wrt immediately preceeding focus event so this should be  ok.
Comment 10 alexander :surkov 2013-03-21 21:39:38 PDT
then it makes change events order in the test
Comment 11 Trevor Saunders (:tbsaunde) 2013-03-21 22:00:26 PDT
(In reply to alexander :surkov from comment #10)
> then it makes change events order in the test

wouldn't it be better  to make them asyncInvokerCheckers since they can technically race?
Comment 12 alexander :surkov 2013-03-21 22:02:16 PDT
up to you
Comment 13 Trevor Saunders (:tbsaunde) 2013-03-21 22:07:27 PDT
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0a2d1bfae2b4
Comment 14 Trevor Saunders (:tbsaunde) 2013-03-22 02:04:02 PDT
kill more expected assertion stuff
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/08c2a05be2e4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3825fdbcec62

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