Lorien Hu (:lsocks) Assignee
	Description • 9 years ago

      No description provided.

	Trevor Saunders (:tbsaunde)
	Comment 2 • 9 years ago

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.

	Lorien Hu (:lsocks) Assignee
	Comment 3 • 9 years ago

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.

	Trevor Saunders (:tbsaunde)
	Comment 4 • 9 years ago

(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.

	Trevor Saunders (:tbsaunde)
	Comment 7 • 9 years ago

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.

	Pulsebot
	Comment 8 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 9 • 9 years ago

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=16019012&repo=mozilla-inbound

	Pulsebot
	Comment 10 • 9 years ago

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cce5dbdf4c7a

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 11 • 9 years ago

Morphing NI to Trevor while Lorien is away.

	Trevor Saunders (:tbsaunde)
	Comment 12 • 9 years ago

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?

	Lorien Hu (:lsocks) Assignee
	Comment 13 • 8 years ago

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.

	Trevor Saunders (:tbsaunde)
	Comment 14 • 8 years ago

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?

	Lorien Hu (:lsocks) Assignee
	Comment 15 • 8 years ago

> 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.

	Trevor Saunders (:tbsaunde)
	Comment 16 • 8 years ago

(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.

	Lorien Hu (:lsocks) Assignee
	Comment 17 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1243916a81e

	Pulsebot
	Comment 18 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 19 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/704276f7f777