358415 - Crash [@ nsHTMLCSSUtils::GetCSSInlinePropertyBase] when inputing title of new blog entry on blogger.com before page is entirely loaded up

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Antoine Bruguier]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20061010 Firefox/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20061010 Firefox/2.0

I think it is related to bug #353704, except I think I have much more detail. This bug occurred with versions 1.5.x and is still present on version 2.x for Windows XP Home.

If you have an account on blogger.com, you can add new entries to your posts my clicking on the + icon. When you do so, a new page that contains input fields for the title and main text of the entry loads up. If you wait for the loading to be done, everything is fine, but if you are impatient and click on the title field and start typing while the page is loading, Firefox crashes.

I can reproduce it sometimes, it all depends on how fast you are. My hunch is that it's a JavaScript issue, and I hope it's not a security risk.

Reproducible: Sometimes

Steps to Reproduce:
1. Create an account with blogger.com and choose to stay signed on and create one blog
2. go to: http://www.blogger.com/home, you should see a page titled "Dashboard" with the list of blogs you have. To the right of each blog name, there should be a "+" icon
3. click on the icon
4. while the page is loading, click on the field for the title and start typing. Timing is *essential*, if you are too slow, it will not crash

Actual Results:  
The Firefox program crashes, Windows asks if I want to send a report (I said yes), and the reporting program asks to send an information (I submitted it too).

Expected Results:  
I expect Firefox not to crash and my title to be inputted.

I hope I was clear, please contact me if you need additional information:
http://www.bruguier.com/contact.html

I checked the 'security problem' box just in case. I am *not* a security expert, but I have heard that JavaScript is a way to infect machines with crashed or buffer overflows... Once again, I am not knowledgeable enough to tell for sure, so I ticked it. Better safe than sorry.

Finally, I rated it as "critical" because it crashes. This problem is limited to blogger.com, to my knowledge.

Comment 1

[Martijn Wargers (dead)]

Ok, I get this talkback ID: TB25363976Z
nsHTMLCSSUtils::GetCSSInlinePropertyBase  [mozilla\editor\libeditor\html\nshtmlcssutils.cpp, line 576]
0x034592a8
0xc2800040
0x64303041

I'll attach a testcase that gives the same kind of crash.

I can see this crash even in Mozilla1.7, so it doesn't seem like a regression.

Not sure if this bug should be security sensitive.

Comment 2

[Martijn Wargers (dead)]

Attachment: testcase

Comment 3

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

Simple null check should be enough in this case.
Just changing the following line http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/editor/libeditor/html/nsHTMLCSSUtils.cpp&mark=574&rev=#574
to 
if (NS_FAILED(res) || !cssDecl) return res;

That is because nsGlobalWindow::GetComputedStyle may return NS_OK and null 
cssDecl if mDocShell or presshell is null.

Other option would be to return some sort of empty cssDecl if mDocShell or presshell is null.

Will test and make a patch tomorrow.

Comment 4

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

This comment is relevant here:
http://lxr.mozilla.org/seamonkey/source/layout/style/nsComputedDOMStyle.h#317

Comment 5

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

This is #87 crasher in FF2

Comment 6

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

Attachment: proposed patch for branch

Comment 7

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

I propose adding null checks to branch, but for trunk
the initialization of nsIComputedDOMStyle could be changed a bit so that
GlobalWindow wouldn't have to have mDocShell and ::GetComputedStyle
wouldn't ever return NS_OK if the returned object is null.

DOM spec sucks, it doesn't handle any error cases, so there aren't
any exceptions defined for getComputedStyle.

And btw, as far as I see, this is just a null pointer crash.

Comment 8

[Boris Zbarsky [:bzbarsky]]

If there's no docshell, there's no presshell either, so we should just throw, imho....

And yes, I'm cool with getComputedStyle throwing something like NS_ERROR_NOT_AVAILABLE if there's nothing in the window.

Comment 9

[Martijn Wargers (dead)]

Olli, you want the proposed patch for branch get reviewed.
Should this bug stay security sensitive?

Comment 10

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

(In reply to comment #8)
> And yes, I'm cool with getComputedStyle throwing something like
> NS_ERROR_NOT_AVAILABLE if there's nothing in the window.
> 
But that would change the API a bit. I wonder if some sites would break 
because of that.

And this is null pointer crash, so shouldn't be security sensitive, I think.

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 244458 [details] [diff] [review]
proposed patch for branch

r+sr=jst, and requesting approval for the branches.

Comment 12

[Jay Patel [:jay]]

Comment on attachment 244458 [details] [diff] [review]
proposed patch for branch

Approved for both branches, a=jay for drivers.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Yeah, this is not really an exploitable crash.  At least not without relying on OS bugs...

Comment 14

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

Checked in to trunk. Will check in to branches ASAP.

Comment 15

[Carsten Book [:Tomcat]]

Verify fixed for 1.8.0.10 and 1.8.1.2 with Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.2pre) Gecko/2007011903 BonEcho/2.0.0.2pre and Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.0.10pre) Gecko/20070120 Firefox/1.5.0.10pre ID:2007012008