667512 - Crash [@ nsIFrame::PeekOffset] with removing br and table cell and pressing right arrow key

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which crashes current trunk build after pressing the right arrow key.

https://crash-stats.mozilla.com/report/index/9bc6ca38-beec-412b-bd06-161462110627
0 		@0x6c0070 	
1 	xul.dll 	nsIFrame::PeekOffset 	layout/generic/nsFrame.cpp:5598
2 	xul.dll 	nsFrameSelection::MoveCaret 	layout/generic/nsSelection.cpp:1240
3 	xul.dll 	nsFrameSelection::MoveCaret 	layout/generic/nsSelection.cpp:1105
4 	xul.dll 	nsFrameSelection::CharacterMove 	layout/generic/nsSelection.cpp:2191
5 	xul.dll 	PresShell::CharacterMove 	layout/base/nsPresShell.cpp:3150
6 	xul.dll 	nsSelectionMoveCommands::DoCommand 	editor/libeditor/base/nsEditorCommands.cpp:718
7 	xul.dll 	nsControllerCommandTable::DoCommand 	embedding/components/commandhandler/src/nsControllerCommandTable.cpp:191
8 	xul.dll 	nsBaseCommandController::DoCommand 	embedding/components/commandhandler/src/nsBaseCommandController.cpp:169
9 	xul.dll 	nsXBLPrototypeHandler::DispatchXBLCommand 	content/xbl/src/nsXBLPrototypeHandler.cpp:499
10 	xul.dll 	nsXBLPrototypeHandler::ExecuteHandler 	
11 	xul.dll 	nsXBLPrototypeHandler::ModifiersMatchMask 	content/xbl/src/nsXBLPrototypeHandler.cpp:1027
12 	xul.dll 	nsCOMPtr_base::assign_from_helper 	obj-firefox/xpcom/build/nsCOMPtr.cpp:152
13 	xul.dll 	nsCOMPtr<nsIDOMElement>::nsCOMPtr<nsIDOMElement> 	obj-firefox/dist/include/nsCOMPtr.h:644
14 	xul.dll 	nsXBLWindowKeyHandler::GetElement 	content/xbl/src/nsXBLWindowKeyHandler.cpp:589
15 	xul.dll 	nsCOMPtr_base::assign_with_AddRef 	obj-firefox/xpcom/build/nsCOMPtr.cpp:88
16 	xul.dll 	nsRefPtr<nsIDOMEventListener>::~nsRefPtr<nsIDOMEventListener> 	obj-firefox/dist/include/nsAutoPtr.h:969
17 	xul.dll 	nsHashtable::Get 	xpcom/ds/nsHashtable.cpp:246
18 	xul.dll 	nsXBLDocumentInfo::GetPrototypeBinding 	content/xbl/src/nsXBLDocumentInfo.cpp:545
19 	xul.dll 	nsXBLDocumentInfo::GetPrototypeBinding 	content/xbl/src/nsXBLDocumentInfo.cpp:545
20 	xul.dll 	nsXBLSpecialDocInfo::GetHandlers 	content/xbl/src/nsXBLWindowKeyHandler.cpp:153
21 	xul.dll 	nsXBLSpecialDocInfo::GetAllHandlers 	content/xbl/src/nsXBLWindowKeyHandler.cpp:173

Comment 1

[Martijn Wargers (dead)]

Hg blame isn't useful at nsFrame.cpp:5598 (as too often the case with hg blame, unfortunately.

I get a regression range between 20101104 and 20110105. I don't have any builds inbetween, atm.

Comment 2

[Marcia Knous [:marcia]]

I moved the crash stack to the crash signature field - [@ nsIFrame::PeekOffset(nsPeekOffsetStruct*) ] is the only thing we want in that field.

Comment 3

[Thomas Ahlblom]

WFM:
Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.2.18) Gecko/20110614 Firefox/3.6.18

Reproduced:
Mozilla/5.0 (X11; Linux x86_64; rv:5.0.1) Gecko/20100101 Firefox/5.0.1
Mozilla/5.0 (X11; Linux x86_64; rv:6.0) Gecko/20100101 Firefox/6.0
Mozilla/5.0 (X11; Linux x86_64; rv:7.0a2) Gecko/20110723 Firefox/7.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:8.0a1) Gecko/20110723 Firefox/8.0a1

Last good nightly: 2010-11-14
First bad nightly: 2010-11-15

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=674f2ed15cea&tochange=edf41ff32f08

Linux crashes:
bp-90b7715d-9f9b-4710-9953-6634f2110723
bp-3242ba3f-b275-4e8e-b853-267d92110723
bp-76ae7ef4-65cd-49be-965f-262132110723
bp-4f371a10-ff90-4fe3-b45e-272562110723
bp-2a1a42d2-3f75-4eee-a51a-c00e02110723
bp-b517a0c1-6d3b-4230-bca5-e63792110723
bp-321bee19-d34b-40fc-b91d-a7fd02110723
bp-b46e14a6-a14b-498e-811e-2199e2110723
bp-b61ad8f2-14c2-4c00-ae66-1f7902110723
bp-22806048-4d0e-4994-9938-2566e2110723
bp-431e64d3-f6e8-4cba-a7f7-723c02110723

Comment 4

[Thomas Ahlblom]

Please ignore bp-90b7715d-9f9b-4710-9953-6634f2110723 (the first Report ID) in comment 3. It's probably unrelated to this bug.

Comment 5

[Thomas Ahlblom]

Local track down:

The first bad revision is:
changeset:   57481:54d00e3411ab
user:        Ehsan Akhgari <[e-mail]>
date:        Thu Nov 11 16:40:52 2010 -0500
summary:     Bug 611182 - Part 3: Handle dynamic changes to the editable documents and create/remove the bogus node if needed; r=bzbarsky a=blocking-beta8+

Comment 6

[(no longer active)]

Hmm, this patch has been backed out, so I don't know how it could cause a crash in current nightlies.  I'll take a look though.

Comment 7

[(no longer active)]

Attachment: Patch (v1)

The patch is pretty simple, we should just check to see if we have any visual frames.

Comment 8

[(no longer active)]

Oh, and this is most certainly not a regression from bug 611182!

Comment 9

[(no longer active)]

This affects Firefox 4, 5, 6, 7 and 8 (trunk).

We're returning the contents of stack memory as an nsIFrame*.  Which means that a carefully engineered web page can probably trigger this in a way that would be exploitable.

Comment 10

[Daniel Veditz [:dveditz]]

generally assume EXCEPTION_ACCESS_VIOLATION_EXEC is exploitable unless you're 100% sure both 1) an attacker can't influence the address AND 2) there's no way for an attacker to put data at the address.

The fix is deceptively simple. Why did this suddenly appear to break on Nov 14 2010? The only change that seems at all relevant to the testcase is bug 595758. That one is guarding against a null-deref and if that path were coming up you'd be crashing due to that bug, not this.

Comment 11

[Thomas Ahlblom]

Now I've redone the track down, and once again I hit the same changeset as in comment 5, so it must have been some kind of local regression. However, I also noticed that I had to press right arrow twice to trigger the crash near the regression.

Anyway, how do I find out if and when a patch has been backed out? (using the hg command or http://hg.mozilla.org/ ?) It could be interesting to see if we can find a good Nightly after that.

Comment 12

[(no longer active)]

(In reply to comment #10)
> generally assume EXCEPTION_ACCESS_VIOLATION_EXEC is exploitable unless
> you're 100% sure both 1) an attacker can't influence the address AND 2)
> there's no way for an attacker to put data at the address.
> 
> The fix is deceptively simple. Why did this suddenly appear to break on Nov
> 14 2010? The only change that seems at all relevant to the testcase is bug
> 595758. That one is guarding against a null-deref and if that path were
> coming up you'd be crashing due to that bug, not this.

Smontagu could say for sure, I don't know this code very well.  But it's quite possible that the bug has been there for some time, we just didn't have a testcase for it.

(In reply to comment #11)
> Anyway, how do I find out if and when a patch has been backed out? (using
> the hg command or http://hg.mozilla.org/ ?) It could be interesting to see
> if we can find a good Nightly after that.

Your best bet is to look at the bug to see if it has been reopened.  hg doesn't give you that information easily...

Comment 13

[(no longer active)]

Hmm, this might actually be a regression from bug 610638.  Its patch falls into the regression range, and it touches the guilty functions here: <http://hg.mozilla.org/mozilla-central/rev/7034c271ebd2>.

Comment 14

[Simon Montagu :smontagu]

(In reply to comment #13)
> Hmm, this might actually be a regression from bug 610638.  Its patch falls
> into the regression range, and it touches the guilty functions here:
> <http://hg.mozilla.org/mozilla-central/rev/7034c271ebd2>.

Do you mean that? That patch just makes some prefs localizable.

Comment 15

[Simon Montagu :smontagu]

Comment on attachment 548317 [details] [diff] [review]
Patch (v1)

Review of attachment 548317 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 16

[(no longer active)]

(In reply to comment #14)
> (In reply to comment #13)
> > Hmm, this might actually be a regression from bug 610638.  Its patch falls
> > into the regression range, and it touches the guilty functions here:
> > <http://hg.mozilla.org/mozilla-central/rev/7034c271ebd2>.
> 
> Do you mean that? That patch just makes some prefs localizable.

No, you're right, I think I confused that with another changeset.  Honestly, I'm not sure what has regressed this any more...

Comment 17

[(no longer active)]

Landed without the testcase on trunk:

http://hg.mozilla.org/mozilla-central/rev/029dd90bd20e

Comment 18

[LegNeato]

Comment on attachment 548317 [details] [diff] [review]
Patch (v1)

Approved for mozilla-beta and mozilla-aurora. Please land asap.

Comment 19

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/97f7fcf40ee9
http://hg.mozilla.org/releases/mozilla-beta/rev/e9324b64603b

Comment 20

[u279076]

qa+ for verification on Firefox 7

Comment 21

[Al Billings [:abillings - ex-MoCo]]

Crash does not reproduce in Firefox 6.0.2 or 7 on OS X or Windows 7.

Comment 22

[u279076]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0) Gecko/20100101 Firefox/7.0 ID:20110922153450
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a2) Gecko/20110926 Firefox/8.0a2 ID:20110926042011
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0 ID:20110922153450
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0a2) Gecko/20110926 Firefox/8.0a2 ID:20110926042011
Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20100101 Firefox/7.0 ID:20110922153450
Mozilla/5.0 (X11; Linux x86_64; rv:8.0a2) Gecko/20110926 Firefox/8.0a2 ID:20110926042011

Verified fixed using the attached testcase.

Comment 23

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7f8058b2b21

Comment 24

[Ed Morley [:emorley]]

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