485132 - Audit the rest of our Gecko pointer dereferences

Status: RESOLVED INCOMPLETE

(Camino Graveyard :: General, defect)

Description

[Chris Lawson (gone)]

After bug 485126 and bug 480797, I think it would probably be prudent to audit the rest of our Gecko pointer dereferences to make sure we don't have any other potential crashes lurking in our code. Xcode says there are 1470 occurrences of "->" in code that it can search.

I've already started looking through them and can continue doing this a little at a time.

Comment 1

[Chris Lawson (gone)]

OK, I've completed the audit. Here are the findings that looked suspicious; they may not all be problems:

backMenu: and forwardMenu: in BrowserWindowController can have sessionHistory == null.

completeDefaultResult in AutoCompleteTextField assumes mResults will always exist.

showPopupsWhitelistingSource: in BrowserWrapper assumes mBlockedPopups will always exist.

initWithFrame: in CHBrowserView can have a null |browser|, so |_webBrowser| and |baseWin| can both be null. Also in the same method, the line that sets |pref| is assumed to always succeed.

loadURI:referrer:flags:allowPopups: in CHBrowserView will have a null |nav| if _webBrowser is null.

|focusController|, |docShell|, and |contentViewer| in CHBrowserView can return null. The callers to all three *do* appear to null-check as they should.

currentCharset in CHBrowserView looks like bad things will happen if _webBrowser is null.

CHBrowserListener::CreateChromeWindow doesn't null-check |listener| properly (the #debug section just above the first use of listener is written wrong),

selectOption in CHSelectHandler doesn't null-check domDocument, which could lead to null docEvent.

SecurityDialogs::AlertDialog doesn't null-check |pref| at the end.

certificateIsInParentChain in CertificateItem will continue after finding thisCert to be null, and then subsequently uses thisCert without again null-checking it.

traverseSequence in CertificateItem does basically the same thing, but with thisObject.

filledAutoCompleteText in FormFillController -- is event null-safe? We check privateEvent but not event.

handleKeyNavigation: in FormFillController assumes mFocusedInputElement will always exist.

mPrompt in KeychainService.mm is assumed to exist throughout the file.

KeychainFormSubmitObserver::Notify uses |node| when it could be null (line 1066)

fillDOMWindow: uses formElement, usernameElement, passwordElement after possible nulls (same continue situation as CertificateItem above).

attachToInput: in KeychainAutoCompleteSession assumes mDOMListener will always exist.

EncryptString(), DecryptString(), and WLLT_ChangePassword() in wallet.mm all assume gSecretDecoderRing will always exist. This may not be a problem; I'm not sure if it's always created when initing wallet code.

loadKeychainData in KeychainItem uses attrList without a check (twice).

establishSecureKeyWithDataProvider in SafeBrowsingListManager assumes mListManager will always exist.

nsHistoryMdbTableEnumerator::HasMoreElements() assumes mCursor will always exist.

nsHistoryItem::GetID assumes mRow will always exist.

nsSimpleGlobalHistory::AddURI() uses gPrefBranch without check (3x) (probably OK, since Init() guarantees gPrefBranch).

nsSimpleGlobalHistory::RemoveMatchingRows() null-checks row and continues, using it without a check later.

nsSimpleGlobalHistory::CheckHostnameEntries assumes mTable will always exist.

nsSimpleGlobalHistory::FindRow() does too.

HistoryAutoCompleteEnumerator::IsResult() assumes mHistory will always exist.

nsSimpleGlobalHistory::AutoCompleteGetExcludeInfo assumes mSchemePrefixes and mHostNamePrefixes always exist.

nsSimpleGlobalHistory::AutoCompleteCutPrefix has the same problem.


Some of these may not be a big deal (in particular, a number of the various member variable assumptions may be valid) but several of them are obviously wrong.

I'd appreciate a review of this comment by either Stuart, Pink, or both.

Comment 2

[Chris Lawson (gone)]

Attachment: queue placeholder patch

Stuart and pink, please see comment 1.

Comment 3

[Stuart Morgan]

(In reply to comment #1)
> backMenu: and forwardMenu: in BrowserWindowController can have sessionHistory
> == null.

Check, conditionalizing everything through popuplateSessionHistoryMenu:...

> completeDefaultResult in AutoCompleteTextField assumes mResults will always
> exist.

Check.

> showPopupsWhitelistingSource: in BrowserWrapper assumes mBlockedPopups will
> always exist.

If it doesn't we have another problem, but it should still check.

> initWithFrame: in CHBrowserView can have a null |browser|, so |_webBrowser| and
> |baseWin| can both be null.

Only if do_CreateInstance(NS_WEBBROWSER_CONTRACTID, &rv) just failed, in which case the world is probably ending anyway. Bullet-proofing wouldn't hurt, but would require a very careful restructuring of destroyWebBrowser so that things wouldn't be leaked by a |[self autorelease]; return nil;| (the CHSelectHandler failure case is already wrong).

> Also in the same method, the line that sets |pref|
> is assumed to always succeed.

Check.

> loadURI:referrer:flags:allowPopups: in CHBrowserView will have a null |nav| if
> _webBrowser is null.

Probably can't happen, but won't hurt to check. Check nav, since that's what matters (do_QueryInterface can return null, so you should never assume that non-null in = non-null out; check what you actually need to null-check, not the input).

> currentCharset in CHBrowserView looks like bad things will happen if
> _webBrowser is null.

Check (early return style please, rather than more nesting).

> CHBrowserListener::CreateChromeWindow doesn't null-check |listener| properly
> (the #debug section just above the first use of listener is written wrong),

Indeed.

> selectOption in CHSelectHandler doesn't null-check domDocument, which could
> lead to null docEvent.

Check docEvent.

> SecurityDialogs::AlertDialog doesn't null-check |pref| at the end.

Check.

> certificateIsInParentChain in CertificateItem will continue after finding
> thisCert to be null, and then subsequently uses thisCert without again
> null-checking it.

No it doesn't; perhaps you are misunderstanding what |continue| does?

> traverseSequence in CertificateItem does basically the same thing, but with
> thisObject.

Same.

> filledAutoCompleteText in FormFillController -- is event null-safe? We check
> privateEvent but not event.

privateEvent is a QI of event, so if event is null the privateEvent check will return early.

> handleKeyNavigation: in FormFillController assumes mFocusedInputElement will
> always exist.

Checked by its caller, which I think is reasonably in this case.

> mPrompt in KeychainService.mm is assumed to exist throughout the file.

Check.

> KeychainFormSubmitObserver::Notify uses |node| when it could be null (line
> 1066)

Check, although if a form node isn't an nsIContent we're in bizarro land.

> fillDOMWindow: uses formElement, usernameElement, passwordElement after
> possible nulls (same continue situation as CertificateItem above).

Same answer.

> attachToInput: in KeychainAutoCompleteSession assumes mDOMListener will always
> exist.

Unless we ran out of memory, it does, but I guess you can check.

> EncryptString(), DecryptString(), and WLLT_ChangePassword() in wallet.mm all
> assume gSecretDecoderRing will always exist.

They call wallet_CryptSetup and check success, so we're fine.

> loadKeychainData in KeychainItem uses attrList without a check (twice).

Because we check the return value from SecKeychainItemCopyAttributesAndData.

> establishSecureKeyWithDataProvider in SafeBrowsingListManager assumes
> mListManager will always exist.

Check.

> nsHistoryMdbTableEnumerator::HasMoreElements() assumes mCursor will always
> exist.

It does.

> nsHistoryItem::GetID assumes mRow will always exist.

Same.

> nsSimpleGlobalHistory::AddURI() uses gPrefBranch without check (3x) (probably
> OK, since Init() guarantees gPrefBranch).

Should probably have an NS_ENSURE_STATE like the other methods.

> nsSimpleGlobalHistory::RemoveMatchingRows() null-checks row and continues,
> using it without a check later.

See above.

> nsSimpleGlobalHistory::CheckHostnameEntries assumes mTable will always exist.
> nsSimpleGlobalHistory::FindRow() does too.

Check.

> HistoryAutoCompleteEnumerator::IsResult() assumes mHistory will always exist.

Looks fine.

> nsSimpleGlobalHistory::AutoCompleteGetExcludeInfo assumes mSchemePrefixes and
> mHostNamePrefixes always exist.
>
> nsSimpleGlobalHistory::AutoCompleteCutPrefix has the same problem.

That's fine.

Comment 4

[Chris Lawson (gone)]

Attachment: fix

Ilya, can you take a gander at this? This fixes everything in comment 3, and I *think* it does the right thing with |destroyWebBrowser|, but I'm not 100% sure.

Comment 5

[Ilya Sherman]

Comment on attachment 382225 [details] [diff] [review]
fix

>   NS_ASSERTION([self popupsBlocked], "no popups to unblock!");
>+  NS_ASSERTION(mBlockedPopups, "mBlockedPopups is null!");

Note that [self popupsBlocked] will already return NO if !mBlockedPopups.


>     // Create the web browser instance
>     nsCOMPtr<nsIWebBrowser> browser = do_CreateInstance(NS_WEBBROWSER_CONTRACTID, &rv);
>     if (NS_FAILED(rv)) {
>       // XXX need to throw
>     }

Throwing whatever we need to throw here would probably be good.  The error-handling code should probably also be part of that; at the very least, it should precede |NS_ADDREF(_webBrowser);|.

I'm also not sure about the |destroyWebBrowser| code; I don't really understand the intricacies of that method.


>     CHSelectHandler* selectHandler = new CHSelectHandler();
>+    if (!selectHandler) {

Am I missing something, or is this code checking whether the system has completely run out of memory?  Not that it's bad to check in that case, but it seems like pretty much everything start to fail if that happened...


r- because I think CHBrowserView.mm might still be not quite right, though someone smarter than me should take a look at that.