Fix for bug 1138436 causes NVDA to keep all loaded documents around forever

RESOLVED FIXED in Firefox 39

Status

()

defect
--
major
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: MarcoZ, Assigned: MarcoZ)

Tracking

({regression})

39 Branch
mozilla40
x86
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 unaffected, firefox39+ fixed, firefox40+ fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

Bisection revealed that bug 1138436 causes NVDA to hang on to any document ever loaded since both NVDA and Firefox were started. The exact revision is:
The first bad revision is:
changeset:   233879:4e8054e7ff96
user:        Trevor Saunders <trev.saunders@gmail.com>
date:        Mon Feb 09 11:57:23 2015 -0500
summary:     bug 1138436 - start on proxying IAccessible2 r=surkov, r=davidb

Steps:
1. Run NVDA.
2. Press NVDA-key+Ctrl+Y to open NVDA's Python console.
3. Paste in the following and press Enter to execute:

import treeInterceptorHandler as tih; len(tih.runningTable)

If you don't have any Firefox instances running, or NVDA didn't access any documents of a Firefox instance yet, this should be 0. Otherwise, just note the current number. Perform this after each of the following steps. You can keep the Python Console open.

4. Run Firefox.
5. make sure e10s is *disabled*.
6. With the Nightly Start page open and NVDA having read it, check the above in the Python console. The number should be 1 or 1 higher than before.
7. Press Ctrl+T to open a new tab. Navigate to any site, I used http://www.heise.de/newsticker (my preferred news site).
8. Check the number. It should be 2, or 1 higher than before.
9. Activate any of the links to a news article.
10. Check the number again.

Expected: It should still be the last recorded number. Since you just went from one to the other document within the same tab.
Actual: The number is 1 higher than before.
11. Press Ctrl+W to close the tab.
12. Check the number again.

Expected: It should have dropped. Ideally to the number in step 6.
Actual: It stays at the last recorded value.

This accumulates over time. So even though you may only have 2 r even 1 tab open at the time, after a busy day, NVDA might think you have 100 documents open.

While bisecting this, I found by accident that this does not happen with e10s turned on. E10s on, which is the default for the temp profile for local builds, and which isn't even turned off by accessibility, will not expose this bug.
it could be because Trevor didn't fix get_state (see bug 1138436 comment 8) before landing.
(In reply to alexander :surkov from comment #1)
> it could be because Trevor didn't fix get_state (see bug 1138436 comment 8)
> before landing.

I'm not sure what you mean "fix" it seems to return the DEFUNCT state though it also returns an error code instead of S_OK.    I suppose that might be the issue though it seems fishy to me nvda relies on that instead of using events.

So what information can you see about documents that should have gone away? are they just a document without children where all methods return errors, or do they still have content?
(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> I'm not sure what you mean "fix" it seems to return the DEFUNCT state though
> it also returns an error code instead of S_OK.   
That'll probably do it. If an error code is returned, the error takes precedence and the return value is ignored, which is correct behaviour. However, an error doesn't necessarily mean the object is dead, so we don't assume this.

> I suppose that might be
> the issue though it seems fishy to me nvda relies on that instead of using
> events.
You can't reliably use events to detect object death in the MSAA world, since you don't always know the event parameters for an object you retrieved. Also, event parameters don't necessarily retrieve the same object in every case. Neither of these applies to Firefox, but like it or not, Firefox isn't the only server we deal with.
This fixes the problem.
Assignee: nobody → mzehe
Status: NEW → ASSIGNED
Attachment #8599753 - Flags: review?(surkov.alexander)
Attachment #8599753 - Flags: review?(surkov.alexander) → review+
(In reply to James Teh [:Jamie] from comment #3)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > I'm not sure what you mean "fix" it seems to return the DEFUNCT state though
> > it also returns an error code instead of S_OK.   
> That'll probably do it. If an error code is returned, the error takes
> precedence and the return value is ignored, which is correct behaviour.
> However, an error doesn't necessarily mean the object is dead, so we don't
> assume this.

error codes in general make sense, but I'm not sure what else CO_E_OBJNOTCONNECTED might be used for.

> 
> > I suppose that might be
> > the issue though it seems fishy to me nvda relies on that instead of using
> > events.
> You can't reliably use events to detect object death in the MSAA world,
> since you don't always know the event parameters for an object you
> retrieved. Also, event parameters don't necessarily retrieve the same object
> in every case. Neither of these applies to Firefox, but like it or not,
> Firefox isn't the only server we deal with.

that makes sense, but bleh ;)
Same as above, but contains a commit message. Alex, because the tree is still closed for me, and I have a public holiday tomorrow, can you land this for me when the tree reopens and also request approval for Aurora (39)? Thanks!
Attachment #8599961 - Flags: checkin?(surkov.alexander)
Tracking for 39 and 40.
(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> error codes in general make sense, but I'm not sure what else
> CO_E_OBJNOTCONNECTED might be used for.
For the record, returning CO_E_OBJNOTCONNECTED is certainly valid. If this were a new implementation, I'd agree, and I'll probably tweak NVDA to handle this case in future. However, Gecko has returned the defunct state (without an error) for many years now, so we came to rely on this. Given that the defunct state is also a valid response and this is a one line patch, it doesn't seem prudent to regress NVDA for a minium of 3 months until we can get another release out with the change (and maybe other ATs as well).
(In reply to James Teh [:Jamie] from comment #8)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> > error codes in general make sense, but I'm not sure what else
> > CO_E_OBJNOTCONNECTED might be used for.
> For the record, returning CO_E_OBJNOTCONNECTED is certainly valid. If this
> were a new implementation, I'd agree, and I'll probably tweak NVDA to handle
> this case in future. However, Gecko has returned the defunct state (without
> an error) for many years now, so we came to rely on this. Given that the
> defunct state is also a valid response and this is a one line patch, it
> doesn't seem prudent to regress NVDA for a minium of 3 months until we can
> get another release out with the change (and maybe other ATs as well).

I wasn't suggesting that. However I do  still find it a little suprising this broke something, and think its rather unfortunate this method is different from all the others in what it does with defunct objects.
(In reply to Trevor Saunders (:tbsaunde) from comment #9)
> However I do  still find it a little suprising
> this broke something, and think its rather unfortunate this method is
> different from all the others in what it does with defunct objects.
It kinda has to be if the defunct state is to have any meaning at all. An error code suggests the return value should be ignored, which means the defunct state would never actually be consumed by the client. You could certainly argue the defunct state is silly and should never have existed in the first place (and I probably wouldn't argue with that).
CO_E_OBJNOTCONNECTED means you cannot rely on the methods retval, in case of IAccessible::state, it returns DEFUCT state and it's S_OK 'cause it returns valid value. You could say that you should treat CO_E_OBJNOTCONNECTED hresult same way as DEFUCT value but it's just a matter of historical reasons. There's no point to change that.
Attachment #8599753 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/97d277305ed0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment on attachment 8599961 [details] [diff] [review]
Patch for landing

Approval Request Comment
[Feature/regressing bug #]: bug 1138436
[User impact if declined]: Each opened tab, each followed link would cause a document to hang around in memory even after closing it, if NVDA or other screen readers are running.
[Describe test coverage new/current, TreeHerder]: On Mozilla-Central and manually verified by me also after writing the patch
[Risks and why]: No risk, this patch just reverts a return value to what it was for longer than this old guard has been with mozilla. :)
[String/UUID change made/needed]: None.
Attachment #8599961 - Flags: approval-mozilla-aurora?
Note: If this gets approved after aurora moved to 40, of course this would mean Beta. The important thing is: Get it into 39.
Comment on attachment 8599961 [details] [diff] [review]
Patch for landing

Looks good for uplift to aurora. Thanks for testing it as well as fixing it!
Attachment #8599961 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Need a check for the aurora-approved patch. Doing this because I received a nag-e-mail about the aproved patch thathasn't been acted upon yet. ;)
Keywords: checkin-needed
It's preferred you not use checkin-needed for uplifts. We have different bug queries for them and it creates extra noise by doing so.
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.