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

Status: RESOLVED FIXED

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

Description

[Gabriele Svelto [:gsvelto] (PTO)]

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.

Comment 1

[Gabriele Svelto [:gsvelto] (PTO)]

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.

Comment 2

[Kevin Liu]

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.

Comment 3

[Matthew N. [:MattN]]

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

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1627673 - Do null-check the result of `DragEvent::GetDataTransfer()` in `EditorEventListener`` r=smaug!

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.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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?

Comment 8

[Matthew N. [:MattN]]

(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

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5e40825560b7
Do null-check the result of `DragEvent::GetDataTransfer()` in `EditorEventListener`` r=smaug

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/5e40825560b7

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR78 approval when you get a chance.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Well, I'll try to do it.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch for ESR78

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
This is crash near null. And the frequency is not rare.
User impact if declined:
So, users may keep see the crash.
Fix Landed on Version:
84
Risk to taking this patch (and alternatives if risky):
Nothing. Just adding a nullptr check.

Comment 15

[Julien Cristau [:jcristau]]

Comment on attachment 9190954 [details] [diff] [review]
Patch for ESR78

nullptr crash fix, approved for 78.6esr

Comment 16

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/fc39ab246aa8