Closed Bug 1199755 Opened 4 years ago Closed 3 years ago

Retrieve remote children correctly from non-remote mozAccessible

Categories

(Core :: Disability Access APIs, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: lsocks, Assigned: lsocks)

References

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
Assignee: nobody → lorien
Attachment #8656744 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8656744 [details] [diff] [review]
part 1 - invalidate and retrieve children correctly

sorry about the delay.

note it would be really nice if you made diffs with -p.


>+++ b/accessible/generic/OuterDocAccessible.cpp	Thu Sep 03 12:12:24 2015 -0700
>@@ -202,6 +202,5 @@
>       return doc;
>   }
> 
>-  MOZ_ASSERT(false, "no top level tab document?");

this should probably in its own patch and be justified with why its ok.

>@@ -782,7 +783,7 @@
> 
>     Accessible* outerDoc = proxy->OuterDocOfRemoteBrowser();
>     nativeParent = outerDoc ?
>-      GetNativeFromGeckoAccessible(outerDoc->RootAccessible()) : nil;
>+      GetNativeFromGeckoAccessible(outerDoc) : nil;

also should be in a separate patch.

>   NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL;
> 
>   AccessibleWrap* accWrap = [self getGeckoAccessible];
>-  if (mChildren || (accWrap && !accWrap->AreChildrenCached()))
>+  if (mChildren || (accWrap && !accWrap->AreChildrenCached())) {
>     return mChildren;
>+  }

also unrelated so different patch please especially since its style only.
Attachment #8656744 - Flags: review?(tbsaunde+mozbugs) → review+
That change

>-      GetNativeFromGeckoAccessible(outerDoc->RootAccessible()) : nil;
>+      GetNativeFromGeckoAccessible(outerDoc) : nil;

is necessary for this bug to be fixed since Mac accessibility will not recognize children if the parent of the child and the parent's child are not the same. If we get the root accessible the parent's parent is returned instead.
(In reply to Lorien Hu (:lsocks) from comment #3)
> That change
> 
> >-      GetNativeFromGeckoAccessible(outerDoc->RootAccessible()) : nil;
> >+      GetNativeFromGeckoAccessible(outerDoc) : nil;
> 
> is necessary for this bug to be fixed since Mac accessibility will not
> recognize children if the parent of the child and the parent's child are not
> the same. If we get the root accessible the parent's parent is returned
> instead.

fixing it is totally fine, just put in a different patch because while its related its a different issue from the cache stuff.
Depends on: 1214316
Attached patch up-to-date (obsolete) — Splinter Review
Attachment #8673246 - Flags: review?(tbsaunde+mozbugs)
Attached patch up-to-date (obsolete) — Splinter Review
Attachment #8673246 - Attachment is obsolete: true
Attachment #8673246 - Flags: review?(tbsaunde+mozbugs)
Attachment #8673247 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8673247 [details] [diff] [review]
up-to-date

I still think this is fixing a couple separate issues (the invalidation stuff in Platform.mm, the parent stuff and the child stuff) but meh I guess it'll be ok, and separating out the assert change is the important part.
Attachment #8673247 - Flags: review?(tbsaunde+mozbugs) → review+
Keywords: checkin-needed
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=16019012&repo=mozilla-inbound
Flags: needinfo?(lorien)
Morphing NI to Trevor while Lorien is away.
Flags: needinfo?(lorien) → needinfo?(tbsaunde+mozbugs)
so, we grab the element that owns the TabParent and that's fine there is an element, but when we try to get a DocAccessible that contains that element we  don't find one.  Given that the element is xul I wonder if OwnerDoc() is the wrong doc, as in some xbl one not the DocAccessible for browser.xul which I'm pretty sure should have a DocAccessible whenever a TabParent is around.  Smaug thoughts? can you take a quick look somehow?
Flags: needinfo?(tbsaunde+mozbugs)
The assertion fails because the outer doc of the remote browser has already been marked for destruction. When it's destroyed, the contents in child are then destroyed, sending a shutdown event back to the parent. When we get the ProxyDestroyed then we are trying to fetch a parent that we've already shutdown.
Attachment #8656744 - Attachment is obsolete: true
Attachment #8673247 - Attachment is obsolete: true
Attachment #8737681 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8737681 [details] [diff] [review]
patch with assertion fix

># HG changeset patch
># User Lorien Hu <lorien@lorienhu.com>
># Date 1459740674 25200
>#      Sun Apr 03 20:31:14 2016 -0700
># Node ID ec827d398577e98406a2b06ab817b2e0b3b589d3
># Parent  cc8a4c0bb7e23906ff0b404f2e1c35aea6ca1279
>Bug 1199755 - Retrieve remote children correctly from non-remote mozAccessible

a longer committ message that explains what's going on would be nice.

>   uintptr_t accWrap = reinterpret_cast<uintptr_t>(aProxy) | IS_PROXY;
>   mozAccessible* mozWrapper = [[type alloc] initWithAccessible:accWrap];
>   aProxy->SetWrapper(reinterpret_cast<uintptr_t>(mozWrapper));
>+
>+  // Invalidate native parent in parent process's children on proxy creation
>+  if (aProxy->IsDoc() && aProxy->AsDoc()->IsTopLevel()) {
>+    Accessible* outerDoc = aProxy->OuterDocOfRemoteBrowser();
>+    if (outerDoc) {
>+      mozAccessible* nativeParent = GetNativeFromGeckoAccessible(outerDoc);
>+      [nativeParent invalidateChildren];
>+    }
>+  }
> }

why is it only necessary to invalidate the parent's children when the parent is an outer doc?  It seems like that should happen for all mozAccessible objects with a parent?


> 
> void
> ProxyDestroyed(ProxyAccessible* aProxy)
> {
>+  // Invalidate native parent in parent process's children on proxy destruction
>+  if (aProxy->IsDoc() && aProxy->AsDoc()->IsTopLevel()) {
>+    Accessible* outerDoc = aProxy->OuterDocOfRemoteBrowser();
>+    if (outerDoc) {
>+      mozAccessible* nativeParent = GetNativeFromGeckoAccessible(outerDoc);
>+      [nativeParent invalidateChildren];
>+    }
>+  }

same

>+        mozAccessible* nativeRemoteChild = GetNativeFromProxy(proxyDoc);
>+        if (nativeRemoteChild) {
>+          [mChildren insertObject:nativeRemoteChild atIndex:0];
>+        } else {
>+          NSLog (@"%@ found a child remote doc missing a native\n", self);

can this be NS_ASSERTION / MOZ_ASSERT or is there a reason not?
> why is it only necessary to invalidate the parent's children when the parent
> is an outer doc?  It seems like that should happen for all mozAccessible
> objects with a parent?

It should, I was going to put that in a different patch (title of this bug is remote from non-remote).

> can this be NS_ASSERTION / MOZ_ASSERT or is there a reason not?

Yeah, I can change that.
(In reply to Lorien Hu (:lsocks) from comment #15)
> > why is it only necessary to invalidate the parent's children when the parent
> > is an outer doc?  It seems like that should happen for all mozAccessible
> > objects with a parent?
> 
> It should, I was going to put that in a different patch (title of this bug
> is remote from non-remote).

ok, I couldn't care less about the name of the bug if the commit does one thing and has a good commit message, but I guess fixing it in another bug is fine.
Attachment #8737681 - Flags: review?(tbsaunde+mozbugs) → review+
Blocks: 1274195
Pushed by lorien@lorienhu.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/704276f7f777
Retrieve native proxy children when mozAccessible children is called on outerdocs in parent r=tbsaunde
https://hg.mozilla.org/mozilla-central/rev/704276f7f777
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.