646662 - ###!!! ASSERTION: This is unsafe! Fix the caller! in nsEventDispatcher

Status: VERIFIED FIXED

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

Description

[Nils]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: 

Description:
The attached testcase triggers following assertion on debug builds:

 ###!!! ASSERTION: This is unsafe! Fix the caller!

A stack back trace at the time of the assertion is attached. Additionally exploitable looking crashes have been observed on Linux, Windows and Mac.

The reduced test case needs a few reloads to crash the browser (especially on windows). The likelihood of crashes can be increased by loading the testcase in several tabs and iframes. Regardless of the crashes, the assertion seem to happen for every reload.

Affected Versions:
Firefox 4.0
Trunk

Testcase:
The testcase is attached as an HTML file. It will crash the browser on opening after several reloads. 

Testcase Notes:
gc() triggers garbage collection. It requires Jesse's quitter extension (https://www.squarefree.com/extensions/quitter.xpi).

Stack Backtrace:

The stack during the first assertion on Linux is attached.

VulnDev reference    : vd11007

reported by nils of vulndev ltd.

Reproducible: Always

Comment 1

[Nils]

Attachment: testcase (crashes browser)

Comment 2

[Nils]

Attachment: gdb output on mac

Comment 3

[Nils]

Attachment: stack during assertion

Comment 4

[Boris Zbarsky [:bzbarsky]]

Should we be popping removable script blockers before calling userdata handlers?

Can we just drop support for userdata?  :(

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I say the latter!

Comment 6

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

Can't reproduce the crash (with quitter.xpi) on 1.9.2 nor
with modified testcase (with domFuzzLite.xpi)
But the assertion is there, and that is serious enough and
fixing the cause for that could hopefully fix the crash too.

Comment 7

[Johnny Stenback (:jst)]

Olli, can you provide a patch that fixes the assertion alone and we'll start there?

Comment 8

[Asa Dotzler [:asa]]

Ollie, we're coming up to the train station and time is short for a fix that's wanted for Firefox 5. Can you give us an update here, please?

Comment 9

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

ETA end of this week

Comment 10

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

The assertion doesn't occur on trunk anymore.
Possibly fixed in Bug 650493.

Comment 11

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

building aurora for testing....

Comment 12

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

I can't get even the assertion now on 1.9.2 (with https://www.squarefree.com/extensions/quitter.xpi)

Will try on aurora once the build is ready.

Comment 13

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

Ok, I can still reproduce the assertion on aurora

Comment 14

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

Attachment: Fixes the assertion

As far as I see, we shouldn't have script blocker in stack when
calling AdoptNode.

On trunk the problem doesn't happen because we use nsUserDataCaller
script runner.

I doubt the patch helps with the crash (which I can't reproduce) though.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 531567 [details] [diff] [review]
Fixes the assertion

I suspect fixing bug 639648 will fix the crash.

Comment 16

[Asa Dotzler [:asa]]

With the crash fix landed is it still sg:crit to get this assertion fixed? What's the risk of taking this fix so late in the Firefox 5 cycle?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Did the patch for bug 639648 fix the crash on fx5?

Comment 18

[Asa Dotzler [:asa]]

Boris, yes, that fix seems to have stopped the crash on Aurora.  Do we still need this assertion fix?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Not sure.  Check with Jonas?

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 531567 [details] [diff] [review]
Fixes the assertion

Checked with Jonas and he feels more comfortable with taking this than not. This patch prevents us from getting into uncharted territory where exploitable things *could* be possible, though we do not at this point know of any specific ways where an actual exploit would happen.

Comment 21

[Johnny Stenback (:jst)]

This is fixed since bug 650493 landed. We just need to land this on the beta branch. Olli, can you do the landing?

Comment 22

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

Yeah, I'll try to do it tomorrow.
(Sorry, I was traveling for few days)

Comment 23

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

I'm having trouble to clone mozilla-beta...

Comment 24

[Asa Dotzler [:asa]]

Ollie, do either of these links help you get your build? 
http://ftp.mozilla.org/pub/mozilla.org/firefox/bundles/ 
https://developer.mozilla.org/En/Developer_Guide/Source_Code/Mercurial#Bundles

Comment 25

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

I'll re-try today. the problem must have been just some networking
error.

Comment 26

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

http://hg.mozilla.org/releases/mozilla-beta/rev/aff078c52e46

Comment 27

[Daniel Veditz [:dveditz]]

Olli: does this patch also for for the 1.9.2 branch or do we need to do something else? code-freeze is Monday June 6

Comment 28

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

I can't reproduce the problem on 192, and the patch doesn't apply cleanly.

But I'll update the patch for 192 anyway.

Comment 29

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

Attachment: for 1.9.2

But to fix the real problem, we need Bug 639648

Comment 30

[LegNeato]

We have Bug 639648 on 1.9.2...does that mean we don't need this? Or do they both need to come in?

Comment 31

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

I think we should take this, but this needs r and a.

Comment 32

[Daniel Veditz [:dveditz]]

Comment on attachment 537187 [details] [diff] [review]
for 1.9.2

Approved for 1.9.2.18, a=dveditz for release-drivers

Please land after getting jonas's OK.

Comment 33

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

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1878344aded9

Comment 34

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

Since there is no STR for 1.9.2 and comment 28 says that the issue cannot be reproduced here on 1.9.2, am I correct in my belief that there is nothing for QA to do here for 1.9.2 verification?

Comment 35

[Trif Andrei-Alin[:AlinT]]

Mozilla/5.0 (Windows NT 6.1; rv:6.0) Gecko/20100101 Firefox/6.0
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.20) Gecko/20110803 Firefox/3.6.20
Runned the testcase and the browser did not crash.
Setting resolution to VERIFIED FIXED.
Thanks!