Daniel Holbert [:dholbert] Reporter
	Description • 9 years ago

      No description provided.

	Daniel Holbert [:dholbert] Reporter
	Comment 1 • 9 years ago

Spinning this bug off for the content-process crash that remains after bug 1124821.

STR:
 1. Be logged into a graphical desktop like Ubuntu Unity or gnome-shell, and run:
       gsettings set org.gnome.desktop.interface toolkit-accessibility true
    to enable a11y and make Firefox detect that it should enable a11y.

 2. In a profile with e10s enabled (e.g. a fresh profile that you're just using for the first time), visit http://www.forbes.com/sites/sarahjeong/2015/01/22/the-dread-pirates-diary/

 3. Click "Continue to site" link at upper-right, on splash page.
 4. Wait ~30 seconds.

ACTUAL RESULTS:
The tab crashes (or maybe aborts?) and shows the tab crashed UI.

I've got a "rr" recording of this that tbsaunde has kindly agreed to poke around in... stay tuned.

	Daniel Holbert [:dholbert] Reporter
	Comment 2 • 9 years ago

Here's a log of what the terminal output looks like (as replayed by "rr"), just before the child process dies.

(I believe the nsStringStats output at the end is an indication that the child process has just died.)

	Daniel Holbert [:dholbert] Reporter
	Comment 3 • 9 years ago

Also, FWIW, we get a bunch of instances of this assertion throughout the recording:
{
###!!! ASSERTION: does it make sense to Show()/Hide() a disabled widget?: 'mEnabled', file widget/PuppetWidget.cpp, line 173
}

I think this is unrelated to the actual issue here, since it's so frequent & happens much much earlier than the child process dying. I'm only bringing it up to say that it's probably ignorable. (whereas the other assertions all seem a11y-related & relevant)

	Daniel Holbert [:dholbert] Reporter
	Comment 5 • 9 years ago

Bug 1138700 seems very similar to this bug, & may be easier to trigger (since it doesn't require interaction).  Just enter Responsive Design Mode (ctrl+shift+m) and visit wired.com, and wait for a second or two.  And have your system configured so that a11y is enabled, per comment 0 here.

tbsaunde / smaug, can you reproduce bug 1138700 locally? If so, maybe you no longer to rely on rr & remote debugging on my machine..

	Daniel Holbert [:dholbert] Reporter
	Comment 6 • 9 years ago

I can't reproduce this or Bug 1138700 in current nightly, actually!  I'll see if I can figured out when it was fixed in a bit.

(sanity-check: I do still have a11y enabled in "gsettings", and Firefox is still running in a11y-enabled mode, as evidenced by its force-disabling e10s after I quit & restart the browser.)

	Daniel Holbert [:dholbert] Reporter
	Comment 7 • 9 years ago

Fix range (using this bug's forbes.com STR) is:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c5b90c003be8&tochange=993eb76a8bd6

which includes:
> db7e1eae0a00	Trevor Saunders — bug 1123511 - temporarily disable ipc accessibility for crashes

I'm betting that's what's saving us here.  So, this isn't really fixed; just hacked around.

	Trevor Saunders (:tbsaunde) Assignee
	Comment 8 • 9 years ago

The code for managing document lifetimes assumed documents could not be
rebound to parents, however that is not the case.

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

Comment on attachment 8582551 [details] [diff] [review]
handle unbinding and rebinding of documents in content processes

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

r=me, nice catch! Nits follow:

::: accessible/base/NotificationController.cpp
@@ +281,5 @@
>      for (size_t i = 0; i < newDocCount; i++) {
>        DocAccessible* childDoc = newChildDocs[i];
> +      Accessible* parent = childDoc->Parent();
> +      DocAccessibleChild* parentIPCDoc = mDocument->IPCDoc();
> +      uint64_t id = reinterpret_cast<uintptr_t>(parent->UniqueID());

I'll trust you that there is always a parent in this context (and you didn't add any risk beyond what we had anyway).

@@ +282,5 @@
>        DocAccessible* childDoc = newChildDocs[i];
> +      Accessible* parent = childDoc->Parent();
> +      DocAccessibleChild* parentIPCDoc = mDocument->IPCDoc();
> +      uint64_t id = reinterpret_cast<uintptr_t>(parent->UniqueID());
> +        MOZ_ASSERT(id);

nit: remove two spaces.

::: accessible/ipc/DocAccessibleParent.cpp
@@ +138,5 @@
>    return true;
>  }
> +
> +bool
> +DocAccessibleParent::RecvBindChildDoc(PDocAccessibleParent* aChildDoc, const uint64_t& aID)

Note to self: as per IRC, I was worried this might not get called before the next WillRefresh but long story short, if it doesn't then we should still be okay (given message order should be preserved, presumably at worst we'll do extra unbind/bind pairs - and hopefully that is unlikely).

::: accessible/ipc/DocAccessibleParent.h
@@ +48,5 @@
> +  virtual bool RecvBindChildDoc(PDocAccessibleParent* aChildDoc, const uint64_t& aID) override;
> +  void Unbind()
> +  {
> +    mParent = nullptr;
> +mParentDoc->mChildDocs.RemoveElement(this);

nit: indent 4 spaces.

	Trevor Saunders (:tbsaunde) Assignee
	Comment 10 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/063b559e525d

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/063b559e525d