crash near null [@mozilla::dom::ContentChild::ProcessingError]

RESOLVED FIXED in Firefox 53

Status

()

Core
Disability Access APIs
--
critical
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: tsmith, Assigned: tbsaunde)

Tracking

(Blocks: 1 bug, {crash, testcase})

Trunk
mozilla54
crash, testcase
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox52 disabled, firefox53 fixed, firefox54 fixed)

Details

(Whiteboard: [fuzzblocker])

Attachments

(3 attachments)

(Reporter)

Description

a year ago
Created attachment 8829966 [details]
log.txt

STR:
0) make sure e10 is enabled
1) enable screen reader
2) set dom.allow_scripts_to_close_windows=true (so the test case can trigger the issue)
3) open test case

Found on Ubuntu 16.04

==6694==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fdcb663fd1a bp 0x7fff887d2430 sp 0x7fff887d2430 T0)
    #0 0x7fdcb663fd19 in mozilla::dom::ContentChild::ProcessingError(mozilla::ipc::HasResultCodes::Result, char const*) /home/worker/workspace/build/src/dom/ipc/ContentChild.cpp:2155:3
    #1 0x7fdcb175df4b in mozilla::ipc::MessageChannel::MaybeHandleError(mozilla::ipc::HasResultCodes::Result, IPC::Message const&, char const*) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2134:5
    #2 0x7fdcb175d1ab in mozilla::ipc::MessageChannel::DispatchSyncMessage(IPC::Message const&, IPC::Message*&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1724:10
    #3 0x7fdcb1759a9d in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1684:17
    #4 0x7fdcb175289a in mozilla::ipc::MessageChannel::ProcessPendingRequest(IPC::Message&&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1486:5
    #5 0x7fdcb175222a in mozilla::ipc::MessageChannel::ProcessPendingRequests(mozilla::ipc::AutoEnterTransaction&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1059:13
    #6 0x7fdcb1753823 in mozilla::ipc::MessageChannel::Send(IPC::Message*, IPC::Message*) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1175:9
    #7 0x7fdcb1ea855a in mozilla::dom::PContentChild::SendAllocateTabId(mozilla::dom::IdType<mozilla::dom::TabParent> const&, mozilla::dom::IPCTabContext const&, mozilla::dom::IdType<mozilla::dom::ContentParent> const&, mozilla::dom::IdType<mozilla::dom::TabParent>*) /home/worker/workspace/build/src/obj-firefox/ipc/ipdl/PContentChild.cpp:4415:21
    #8 0x7fdcb66365b7 in mozilla::dom::ContentChild::ProvideWindowCommon(mozilla::dom::TabChild*, mozIDOMWindowProxy*, bool, unsigned int, bool, bool, bool, nsIURI*, nsAString_internal const&, nsACString_internal const&, bool, bool*, mozIDOMWindowProxy**) /home/worker/workspace/build/src/dom/ipc/ContentChild.cpp:772:3
    #9 0x7fdcb66abf44 in mozilla::dom::TabChild::ProvideWindow(mozIDOMWindowProxy*, unsigned int, bool, bool, bool, nsIURI*, nsAString_internal const&, nsACString_internal const&, bool, bool*, mozIDOMWindowProxy**) /home/worker/workspace/build/src/dom/ipc/TabChild.cpp:1047:12
(Reporter)

Comment 1

a year ago
Created attachment 8829967 [details]
test_case.html
(Reporter)

Updated

a year ago
See Also: → bug 1130734
(Assignee)

Comment 2

a year ago
So, the sequence of events is this.

tab T is created in the child process
DocAccessibleChild d is create in the child process, and a PDocAccessible constructor message is sent to the parent process.
Before the parent process recieves the PDocAccessibleConstructor message for d PBrowser::SendDestroy is sent from the parent process to t in the child process.
Then before the parent recieves the PBrowser::__delete__ message it handles the PDocAccessibleConstructor message for d.  We don't realize the tab is already shutting down and create the DocAccessibleParent normally.
now we can send a message to the child actor for d, and it can arrive after PBrowser::__delete__ has destroyd that actor.

smaug, billm is there a way to know we have already sent the destroy message to a TabChild? Or do you have ideas on other ways to fix this?
Flags: needinfo?(wmccloskey)
Flags: needinfo?(bugs)
Hmm, do we in theory have similar issues with all the protocols PBrowser manages and which are created in child process?

Updated

a year ago
Flags: needinfo?(bugs)

Updated

a year ago
Flags: needinfo?(kchen)
Yeah, in theory this can happen with all protocols managed by PBrowser that construct actors from child process. You can handle this by check mIsDestroyed in TabParent.
Flags: needinfo?(kchen)
In a11y case, where this is probably way more likely, could protocol creation happen in parent?
Child would just notify parent it wants a11y to be enabled, and parent would then create the PDoc* if PBrowser isn't shutting down.
(Assignee)

Comment 6

a year ago
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #4)
> Yeah, in theory this can happen with all protocols managed by PBrowser that
> construct actors from child process. You can handle this by check
> mIsDestroyed in TabParent.

I'm not really sure we have a great option for what to do in RecvPDocAccessibleConstructor, but I guess we can just create the parent actor and mark it as already shutdown and ignore any messages sent to it.

(In reply to Olli Pettay [:smaug] from comment #5)
> In a11y case, where this is probably way more likely, could protocol
> creation happen in parent?
> Child would just notify parent it wants a11y to be enabled, and parent would
> then create the PDoc* if PBrowser isn't shutting down.

that seems rather awkward, the protocol is basically caching information about a document in the child process in the parent process.  So we'd need to store a list of changes to the document since we asked the parent process to construct the actor, and then when we recieved the actor send that list of changes to the parent.  That would probably have some impact on percieved load time to, but I'm not sure how much.
Why would you need to cache anything? You would start from scratch when parent tells that PDocAccessible should be created
I agree with Kan-Ru's suggestion. It's annoying that we have to check mIsDestroyed in so many cases, but there aren't many alternatives that I can think of.
Flags: needinfo?(wmccloskey)
(Assignee)

Comment 9

a year ago
(In reply to Bill McCloskey (:billm) from comment #8)
> I agree with Kan-Ru's suggestion. It's annoying that we have to check
> mIsDestroyed in so many cases, but there aren't many alternatives that I can
> think of.

yeah, me too, I'm about to post a patch doing that.


(In reply to Olli Pettay [:smaug] from comment #7)
> Why would you need to cache anything? You would start from scratch when
> parent tells that PDocAccessible should be created

the problem is what happens when the child recieves the PDocAccessibleConstructor message, at that point the document has a bunch of state we need to send to the parent (parent document, child documents, accessible children etc) and currently we don't have machinary to send that all at once.
(Assignee)

Comment 10

a year ago
Created attachment 8830501 [details] [diff] [review]
fix race between PDocAccessibleConstructor messages and PBrowser::Destroy messages
Attachment #8830501 - Flags: review?(wmccloskey)
Attachment #8830501 - Flags: review?(wmccloskey) → review+

Comment 11

a year ago
Pushed by tsaunders@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6bac4ed9287
fix race between PDocAccessibleConstructor messages and PBrowser::Destroy messages r=billm

Comment 12

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a6bac4ed9287
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Does this need backporting to older branches as well?
Assignee: nobody → tbsaunde+mozbugs
status-firefox52: --- → ?
status-firefox53: --- → ?
Flags: needinfo?(tbsaunde+mozbugs)
(Assignee)

Comment 14

a year ago
Comment on attachment 8830501 [details] [diff] [review]
fix race between PDocAccessibleConstructor messages and PBrowser::Destroy messages

Approval Request Comment
[Feature/Bug causing the regression]:n/a
[User impact if declined]:content process crashes with e10s and a11y enabled
[Is this code covered by automated tests?]:some cover it yes
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:none
[Is the change risky?]:not really
[Why is the change risky/not risky?]:pretty simple change
[String changes made/needed]:none
Flags: needinfo?(tbsaunde+mozbugs)
Attachment #8830501 - Flags: approval-mozilla-aurora?
status-firefox52: ? → disabled
status-firefox53: ? → affected
Comment on attachment 8830501 [details] [diff] [review]
fix race between PDocAccessibleConstructor messages and PBrowser::Destroy messages

Crash fix for e10s/a11y, let's take this for aurora.
Attachment #8830501 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 16

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/6666c83998af
status-firefox53: affected → fixed
Setting qe-verify- based on Trevor's assessment on manual testing needs (see Comment 14) and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.