Closed Bug 1128751 Opened 5 years ago Closed 5 years ago

[e10s] content process crash (abort?) when viewing article on forbes.com, with " ###!!! ASSERTION: no proxy for event!", "adding child to unknown accessible", and "IPDL protocol error: Handler for ShowEvent returned error code"

Categories

(Core :: Disability Access APIs, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
e10s + ---
firefox39 --- fixed

People

(Reporter: dholbert, Assigned: tbsaunde)

References

Details

Attachments

(2 files)

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.
Summary: [e10s] content process crash (abort?) when viewing article on forbes.com, with " ###!!! ASSERTION: no proxy for event!" → [e10s] content process crash (abort?) when viewing article on forbes.com, with " ###!!! ASSERTION: no proxy for event!", "adding child to unknown accessible", and "IPDL protocol error: Handler for ShowEvent returned error code"
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.)
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)
Blocks: e10sa11y2
tracking-e10s: --- → +
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..
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.)
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.
The code for managing document lifetimes assumed documents could not be
rebound to parents, however that is not the case.
Attachment #8582551 - Flags: review?(dbolter)
Assignee: nobody → tbsaunde+mozbugs
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.
Attachment #8582551 - Flags: review?(dbolter) → review+
https://hg.mozilla.org/mozilla-central/rev/063b559e525d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.