485126 - Crash [@ -[CHBrowserView isTextBasedContent] ]

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: current Talkback report

I've seen this crash in Talkback periodically, and I saw it myself the other day.  No real STR, but TB53584737x says "closing a window with 3 tabs opened".

Chris says we're somehow ending up with a nil contentWindow or probably dereferencing nsnull, which is apparently a Source of Bad Things.

[12:59am] cl|zzz: basically, anywhere we call [self contentWindow] in that file, or call |contentWindow| on a CHBrowserView object anywhere else, we need to null-check it.
[01:00am] cl|zzz: either that, or we need [self contentWindow] never to return null 
[01:01am] cl|zzz: and I don't see any way to prevent the latter, so I think we need to do the former
[01:01am] cl|zzz: isImageBasedContent has the same problem (line 1185)
[01:02am] cl|zzz: pageLastModifiedDate is also problematic
[01:08am] cl|zzz: sauron: when you file it, here's the list of calls we need to audit: http://mxr.mozilla.org/mozilla/search?find=%2Fcamino%2F&string=contentWindow

Comment 1

[Chris Lawson (gone)]

From that list, in order:

KeychainService.mm#1378 is safe.
FormFillController.mm#147 is safe.
FormFillController.mm#170 is safe.

CHBrowserView.mm#264 is safe, but line 266 is UNSAFE (because it dereferences piWindow, which will be null if contentWindow is null).

CHBrowserView.mm#411 looks safe, but I'm not 100% certain.

CHBrowserView.mm#583 is safe.
CHBrowserView.mm#590 is safe.
CHBrowserView.mm#609 is safe.

CHBrowserView.mm#627 is UNSAFE.

CHBrowserView.mm#715 is safe.

CHBrowserView.mm#1157 is UNSAFE (and the source of the Talkback report behind this bug).

CHBrowserView.mm#1187 is UNSAFE.

CHBrowserListener.mm#197 is safe.

CHBrowserListener.mm#243 might be unsafe; I'm not sure what happens if you do .get() on nsnull, but my gut says it's bad.

BrowserWrapper.mm#587 is safe, but like the first CHBrowserView example, the next line is UNSAFE because piWindow will be null if contentWindow is null.

BrowserWrapper.mm#1424 is safe.

BrowserWindowController.mm#1106 is safe.

BrowserWindowController.mm#4163 is safe.

BrowserWindowController.mm#5124 is safe.

Comment 2

[Chris Lawson (gone)]

Attachment: fix v1.0

This should fix all the UNSAFE cases in comment 1. I'm not sure how to test it, since we don't know how it was originally triggered, but this at least protects us from dereferencing null pointers in cases where the |contentWindow| method could return null.

Comment 3

[timeless]

assuming you mean a nsCOMPtr<>, .get() will return the null, by itself it's perfectly safe, otoh, if you then use that null pointer, you'd be back here where you started :). happy hunting.

Comment 4

[Stuart Morgan]

Comment on attachment 369222 [details] [diff] [review]
fix v1.0

r=me

Comment 5

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 369222 [details] [diff] [review]
fix v1.0

sr=pink

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Landed on cvs trunk.

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

FYI, we had three of these in this week's b2 (i.e., without this patch) Talkback: TB54046221x, TB53584737x, and TB53992711x.  The first two had comments, if we still care to guess how users may have been hitting this crash:

User Comments	close a window with 3 tabs while the connection failed
User Comments	closing a window with 3 tabs opened

(The third crash also had validateActionBySelector before validateToolbarItem.)

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Patch - what's checked in on MOZILLA_1_8_BRANCH

The trunk patch doesn't totally apply on branch; some hunks did, and I landed them (attached).

If Chris has time to port the CHBrowserView.mm hunk 1 and the BrowserWrapper.mm hunk from his original patch before I spin 1.6.8, we'll take that, too, but as smorgan said, "Some null-checking is better than none" ;)

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(For anyone keeping track, CHBrowserView.mm hunk 4 doesn't actually exist on branch.)

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: MOZILLA_1_8_BRANCH version of the rest of the patch

This is cl's backport of the rest of the patch.  I've compiled it and made sure Camino launches, but that's the extent of my ability to test it.  Stuart, can you please sanity-check the backport before I land it?

Comment 11

[Stuart Morgan]

Comment on attachment 381694 [details] [diff] [review]
MOZILLA_1_8_BRANCH version of the rest of the patch

Yep, looks good.

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

"MOZILLA_1_8_BRANCH version of the rest of the patch" landed on the MOZILLA_1_8_BRANCH in advance of 1.6.8, too.