User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:69.0) Gecko/20100101 Firefox/69.0

Steps to reproduce:

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:69.0) Gecko/20100101 Firefox/69.0
Steps:

1.- Open PoC Attachment
2.- Mousemov in input
3.- Oops Tab crash

Actual results:

Gah. Your tab just crashed.

Expected results:

Change the attribute of an input field of type password to an input field of type text using mouseover.
<input type='password'/> - <input type='text'/>

https://crash-stats.mozilla.org/report/index/88a9f3fc-c996-4099-9d6c-2be120191025

Looks like this crashes in nsFrame::HandleDrag, and looks like it's a near-nullptr crash - presumably what's happened is that changing the type of the input reframed it, which then breaks when trying to use the frame which has been nulled out, or something. I think this is a Layout issue (ie the handler should have been removed/readded when the frame type for the element changed, but I could be wrong... :dholbert, can you check?

(In reply to :Gijs (he/him) from comment #1)

https://crash-stats.mozilla.org/report/index/88a9f3fc-c996-4099-9d6c-2be120191025

Looks like this crashes in nsFrame::HandleDrag

...which is a bit curious, as nothing is being dragged in the STR here.

A nullptr crash seems unlikely to be dangerously exploitable, but given it's a trivially-reproducible content-process crash we probably want to try and get this fixed promptly.

This appears to have been broken for quite a while, actually... mozregression points to August 2017:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b911a4c97fde5d8bdeebfd5d0266ee9f7b9e59b2&tochange=d1c70c20e7b52f7295411343e4dc5db8ee7c92b9

In that pushlog, I notice bug 1389314 seems to be touching input event handling; Olli, might that be related?

(In reply to Jonathan Kew (:jfkthame) from comment #2)

Looks like this crashes in nsFrame::HandleDrag

...which is a bit curious, as nothing is being dragged in the STR here.

Addressing this curiosity: it looks like we call HandleDrag for any mousemove event (and the STR does include mousemove behavior):
https://searchfox.org/mozilla-central/rev/2a355e56c490768be676689de18629485c9d699b/layout/generic/nsFrame.cpp#4253-4259

And HandleDrag simply bails early if it finds that the mousebutton isn't pressed (i.e. it distinguishes movements from true drags internally):
https://searchfox.org/mozilla-central/rev/2a355e56c490768be676689de18629485c9d699b/layout/generic/nsFrame.cpp#4774,4782-4783

So the call to HandleDrag isn't especially surprising for these STR after all.

We're crashing when dereferencing frameselection (when trying to determine if the mouse button is down), here in HandleDrag:

  RefPtr<nsFrameSelection> frameselection = GetFrameSelection();
  bool mouseDown = frameselection->GetDragState();

GetFrameSelection() returns null via this stack:

#0  0x00007fe6d5eca6c0 in nsTextEditorState::GetConstFrameSelection() (this=0x7fe6c7c1b580) at $SRC/dom/html/nsTextEditorState.cpp:1112
#1  0x00007fe6d5db79cb in mozilla::dom::HTMLInputElement::GetConstFrameSelection() (this=0x7fe6c7db8850) at $SRC/dom/html/HTMLInputElement.cpp:2281
#2  0x00007fe6d7a43f1d in nsTextControlFrame::GetOwnedFrameSelection() (this=0x7fe6c7c40040) at $SRC/layout/forms/nsTextControlFrame.cpp:1278
#3  0x00007fe6d788bcb3 in nsIFrame::GetConstFrameSelection() const (this=0x7fe6c7c40378) at $SRC/layout/generic/nsFrame.cpp:7990
#4  0x00007fe6d78977ff in nsIFrame::GetFrameSelection() (this=0x7fe6c7c40378) at $SRC/layout/generic/nsFrame.cpp:7981
#5  0x00007fe6d7897904 in nsFrame::HandleDrag(nsPresContext*, mozilla::WidgetGUIEvent*, nsEventStatus*)

At the innermost level there, in nsTextEditorState::GetConstFrameSelection(), the member-var mSelCon is null, so we return null:

nsFrameSelection* nsTextEditorState::GetConstFrameSelection() {
  if (mSelCon) return mSelCon->GetConstFrameSelection();
  return nullptr;
}

So, why is mSelCon null? Well, the mousemove event triggers an event-handler in the testcase, which does:

 this.setAttribute('type','text')

As part of that setAttribute call, we call HTMLInputElement::HandleTypeChange(), which calls mInputData.mState->SyncUpSelectionPropertiesBeforeDestruction(), which calls UnbindFromFrame(), which sets mSelCon = nullptr here:
https://searchfox.org/mozilla-central/rev/2a355e56c490768be676689de18629485c9d699b/dom/html/nsTextEditorState.cpp#2015

Then we continue servicing the same mousemove event (looping in EventTargetChainItem::HandleEventTargetChain) and we trip over this newly-null pointer as shown in the backtrace here, via the callstack described in comment 4.

Trivial solution: maybe HandleDrag should just add a null check for frameselection and bail gracefully? Or, is there something more sophisticated that we need to do for correctness?

(In reply to Daniel Holbert [:dholbert] from comment #5)

Trivial solution: maybe HandleDrag should just add a null check for frameselection and bail gracefully? Or, is there something more sophisticated that we need to do for correctness?

(To be clear: smaug, this is a question for you. :) I'm piggybacking on the existing needinfo flag.)

Sounds like this might be the same underlying bug as in bug 1511302.

Hmm, but why don't we have a new mSelCon? Are we still using the old frame in layout even though type has already changed?

Yes, we're still using the old frame - nothing has flushed caused a style/layout flush.

If there were some JS that inspected layout/styles after changing the type-attr, then we would presumably flush a frame-reconstuction operation here. But we don't have anything to cause such a flush -- we're simply finishing up servicing the mousemove event (which is the event that triggered the type-attr change in the first place) and we trip over this recently null'ed value.

(Not sure if we it's worth it to add an explicit flush somewhere... If we could just ensure that SyncUpSelectionPropertiesBeforeDestruction and subsequent HandleDrag can interact safely here, e.g. with a null check, then that might be simpler & more efficient.)

Sounds like there's nothing exploitable here?

Correct - this is a null-deref crash.

Currently we flush in some cases, but obviously we don't want to flush all the time when moving mouse.
https://searchfox.org/mozilla-central/rev/11d9c7b7fa82fdfb8ac2a8f0864e9d8d5fe2b926/layout/base/PresShell.cpp#481-482,488,511-513
I wonder what could be used as a flag to trigger flush.

I think this is the same as bug 1593362. We should probably just band-aid with a null-check for now (which seems to be what we arrived on over there). If there end up being correctness issues, we can perhaps sort those out separately.