Closed Bug 1333514 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Disability Access APIs, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- disabled
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: tsmith, Assigned: tbsaunde)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase, Whiteboard: [fuzzblocker])

Attachments

(3 files)

Attached file 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
Attached file test_case.html
See Also: → 1130734
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?
Flags: needinfo?(bugs)
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.
(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)
(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.
Attachment #8830501 - Flags: review?(wmccloskey) → review+
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
https://hg.mozilla.org/mozilla-central/rev/a6bac4ed9287
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Does this need backporting to older branches as well?
Assignee: nobody → tbsaunde+mozbugs
Flags: needinfo?(tbsaunde+mozbugs)
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?
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+
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.