1530519 - Crash with "unknown union type" in PContentParent::SendInvokeDragSession

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Marcia Knous [:marcia]]

This bug is for crash report bp-a8a39ee4-224a-4788-bc65-28f8c0190225.

Seen while looking at Mac 65 crash stats: https://bit.ly/2BRdYOk. This crash appears to be new in 65 and spans all versions of Mac OS. Many of the comments mention copy and pasting text. Perhaps a signature change, not sure since it was not seen in 65 beta.

Here are some comments:

• Podcast concluded. "Beachball" appeared. FF crashed. Recent installation of Bitwarden.
• I have been copying text from a pdf into a WordPress site and these last few times, it keeps kicking me out of FireFox each time.
• Tried to copy/paste from Excel into Google Sheets.
• i was dragging a picture into google slides and the program froze and crashed

Top 10 frames of crashing thread:

0 XUL mozilla::ipc::FatalError ipc/glue/ProtocolUtils.cpp:259
1 XUL .str.1.llvm.2376781057090901720 
2 XUL void mozilla::ipc::WriteIPDLParam<nsTArray<mozilla::dom::IPCDataTransferItem> const&> ipc/glue/IPDLParamTraits.h:145
3 XUL void mozilla::ipc::WriteIPDLParam<nsTArray<mozilla::dom::IPCDataTransfer> const&> ipc/glue/IPDLParamTraits.h:145
4 XUL mozilla::dom::PContentParent::SendInvokeDragSession ipc/ipdl/PContentParent.cpp:2222
5 XUL mozilla::dom::ContentParent::MaybeInvokeDragSession dom/ipc/ContentParent.cpp:4533
6 XUL mozilla::EventStateManager::HandleCrossProcessEvent dom/events/EventStateManager.cpp:1247
7 XUL PLDHashTable::Remove xpcom/ds/PLDHashTable.cpp:637
8 XUL mozilla::EventStateManager::PostHandleEvent dom/events/EventStateManager.cpp:2951
9 XUL mozilla::EventDispatcher::Dispatch xpcom/base/nsCOMPtr.h:328

Comment 1

[Andrew McCreight [:mccr8]]

Something is certainly going wrong with that .str.1.llvm.2376781057090901720 in the signature, but it sounds like there is a specific drag and drop issue, too. Off hand, I'd guess it could be some kind of OOM issue.

Comment 2

[Andrew McCreight [:mccr8]]

It looks like these all have the IPC error "unknown union type".

Comment 3

[Andrew McCreight [:mccr8]]

I found what is probably the older signature before LLVM weirdness by looking for crashes where the protosignature contains PContentParent::SendInvokeDragSession.

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I took a quick look at the code, and maybe I'm missing something but it looks like some of these error cases can create an IPCDataTransferItem but not assign to its data member, leaving it in the invalid default-constructed state which is caught by the assertion.

(Edited to clarify that I didn't mean uninitialized in the dangerous C sense.)

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

evilpie, could this be a regression from bug 1497831.

Comment 6

[Tom S. (please needinfo tschuster)]

Quite possible, this function is extremely convoluted. Some of the faulty error handling was there before as well.

Comment 7

[Tom S. (please needinfo tschuster)]

I think the crash signatures are too broad, because there various other IPC messages like printing etc.

Comment 8

[Andrew McCreight [:mccr8]]

You can search for crashes that match this signature where the protosignature contains SendInvokeDragSession to get a better list of crashes: https://crash-stats.mozilla.com/search/?signature=%3Dmozilla%3A%3Aipc%3A%3AFatalError%20%7C%20.str.1.llvm.2376781057090901720&proto_signature=~SendInvokeDragSession&product=Firefox&date=%3E%3D2019-02-22T15%3A32%3A00.000Z&date=%3C2019-03-01T15%3A32%3A00.000Z&_facets=signature&_sort=-date&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature

Comment 9

[Andrew McCreight [:mccr8]]

That's 99 of the 130 total crashes with this signature in the last week.

Comment 10

[Tom S. (please needinfo tschuster)]

Attachment: Bug 1530519 - Fix error handling in TransferableToIPCTransferable. r?smaug

Comment 11

[Tom S. (please needinfo tschuster)]

I don't know how to reproduce these crashes, but I concur with comment 2 and comment 4, that adding error handling should fix those "unknown union errors".

ConvertToShmem should really do error handling, i.e return Maybe<Shmem>? I am not sure how much of if (!dataAsShmem.IsReadable() || !dataAsShmem.Size<char>()) { is just cargo-culted, especially the IsReadable() part.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe805cb52ee2
Fix error handling in TransferableToIPCTransferable. r=smaug

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/fe805cb52ee2

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval if you're comfortable doing so.

Comment 15

[Tom S. (please needinfo tschuster)]

Comment on attachment 9047938 [details]
Bug 1530519 - Fix error handling in TransferableToIPCTransferable. r?smaug

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: None
• User impact if declined: Crashes
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: We can't reproduce this
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a pretty obvious improvement with a small code change
• String changes made/needed:

Comment 16

[Liz Henry (:lizzard)]

Comment on attachment 9047938 [details]
Bug 1530519 - Fix error handling in TransferableToIPCTransferable. r?smaug

Low risk crash fix, let's take it for beta 14.

Comment 17

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ca92748672f1