Closed Bug 1627673 Opened 8 months ago Closed 15 days ago

Crash in [@ mozilla::dom::DataTransfer::HasType]

Categories

(Core :: DOM: Editor, defect, P2)

Unspecified
All
defect

Tracking

()

RESOLVED FIXED
84 Branch
Tracking Status
firefox-esr78 --- fix-optional
firefox82 --- wontfix
firefox83 --- wontfix
firefox84 --- fixed

People

(Reporter: gsvelto, Assigned: masayuki)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

This bug is for crash report bp-6ee2fda0-d08e-4f64-aaf3-1286c0200404.

Top 10 frames of crashing thread:

0 libxul.so mozilla::dom::DataTransfer::HasType const dom/events/DataTransfer.cpp:333
1 libxul.so mozilla::EditorEventListener::DragEventHasSupportingData const editor/libeditor/EditorEventListener.cpp:970
2 libxul.so mozilla::EditorEventListener::DragOverOrDrop editor/libeditor/EditorEventListener.cpp:876
3 libxul.so mozilla::EditorEventListener::DragEnter editor/libeditor/EditorEventListener.cpp:798
4 libxul.so mozilla::EditorEventListener::HandleEvent editor/libeditor/EditorEventListener.cpp:348
5 libxul.so mozilla::EventListenerManager::HandleEventInternal dom/events/EventListenerManager.cpp:1079
6 libxul.so mozilla::EventTargetChainItem::HandleEventTargetChain dom/events/EventListenerManager.h:354
7 libxul.so mozilla::EventTargetChainItem::HandleEventTargetChain dom/events/EventDispatcher.cpp:638
8 libxul.so mozilla::EventDispatcher::Dispatch dom/events/EventDispatcher.cpp:1055
9 libxul.so mozilla::EventStateManager::FireDragEnterOrExit dom/events/EventStateManager.cpp:4803

Some sort of NULL-pointer access happening on drag'n'drop. Not a new crash, the oldest build is I found is 20200203214717 and it seems to affect all platforms.

Two potentially useful comments in the crashes, this one:

Moving one tab to a different window instance.

And this one:

The damn master password popup window shouldn't interrupt actions like dragging a page to bookmark it. (that caused the crash). Can't you add an option to only have to enter master password once per OS session (instead of browsing session), because many users close and open the browser constantly, and it gets annoying. Also, for the love of god, please make the master password account-bound, not per-each-individual installation of firefox, that's a huge security risk!

Emphasis mine.

Priority: -- → P2

Chiming in as someone who gets hit by this bug multiple times a day, tested on both Firefox release 75 and Firefox Nightly 2020-04-27.

It only manifests if I'm on a call and the WebRTC audio/video sharing popup appears. When that's the case, moving a tab to another window triggers the bug almost immediately if I drag it around the screen.

I hit this crash while dragging the last tab (with URL https://twitter.com/nowthisnews/status/1288506488340860929) from a window on one screen to an existing window on another screen while that video was popped out via picture-in-picture.

macOS 10.15.6 with Nightly 80.0a1

Although I don't understand what happens. The crash occurs here.
https://hg.mozilla.org/mozilla-central/file/e4aeb40526cc5ba0717a0c1e835e1f88fe5d8a01/dom/events/DataTransfer.cpp#l333

But mItems becomes nullptr only by cc. Anyway, we should make a wall for protecting user data in the crashed process.

Assignee: nobody → masayuki
Status: NEW → ASSIGNED

(In reply to Matthew N. [:MattN] from comment #3)

I hit this crash while dragging the last tab (with URL https://twitter.com/nowthisnews/status/1288506488340860929) from a window on one screen to an existing window on another screen while that video was popped out via picture-in-picture.

macOS 10.15.6 with Nightly 80.0a1

I cannot reproduce this bug on Windows with this STR.

I guess that DataTransfer::HasType() is inlined in the opt builds and
actually crashed in EditorEventListener::DragEventHasSupportingData()
at accessing aDragEvent->GetDataTransfer() result without null-check
because DataTransfer::mItems is set to nullptr only by the cycle
collector, but it does not make sense to think that it occurs the STR
in bug 1627673 comment 3.

Therefore, this patch adds null-checks in
EditorEventListener::DragEventHasSupportingData(), but also around
accessing DataTransfer::mItems for avoiding crash after cleared by
the cycle collector.

I have no idea how to test this with automated tests.

https://treeherder.mozilla.org/jobs?repo=try&revision=61e9f39ab41983cd9023b2606c4414dff608dd66
If you still reproduce this bug on your environment, and you have a spare time, could you check whether this is fixed with the above builds?

Flags: needinfo?(mozilla+bmo)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #7)

https://treeherder.mozilla.org/jobs?repo=try&revision=61e9f39ab41983cd9023b2606c4414dff608dd66
If you still reproduce this bug on your environment, and you have a spare time, could you check whether this is fixed with the above builds?

Unfortunately I'm not able to reproduce the original problem with those STR either (using the same Fx profile and external monitor setup). Sorry

Flags: needinfo?(mozilla+bmo)

MattN: No problem, and thank you for your confirmation!

Attachment #9186454 - Attachment description: Bug 1627673 - Do null-check around `DragEvent::GetDataTransfer()` and `DataTransfer::mItems` r=smaug! → Bug 1627673 - Do null-check the result of `DragEvent::GetDataTransfer()` in `EditorEventListener`` r=smaug!
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/5e40825560b7
Do null-check the result of `DragEvent::GetDataTransfer()` in `EditorEventListener`` r=smaug
Status: ASSIGNED → RESOLVED
Closed: 15 days ago
Resolution: --- → FIXED
Target Milestone: --- → 84 Branch
You need to log in before you can comment on or make changes to this bug.