293047 - OOM crash/mlk audit for nsWebBrowser

Status: RESOLVED INCOMPLETE

(Core Graveyard :: Embedding: APIs, defect)

Description

[timeless]

this is an audit, hopefully i didn't miss many spots.
most lines are listed with things that relate to them nearby.

as a hint, SetDocShell will always return NS_OK, but the function that calls it
will crash later if it passes null to SetDocShell, it really should have
returned NS_ERROR_OUT_OF_MEMORY when createInstance failed. In general i've
marked lines that return NS_ERROR_FAILURE instead of NS_ERROR_OUT_OF_MEMORY.
i've also marked a few places that have elses after returns. I'm also marking
some functions (esp relating to mInitInfo because the real callers don't check
the return values from  InitWindow (just as this sucky file doesn't check the
results from mInternalWidget->Create).

Comment 1

[timeless]

Attachment: rewrite logic

i spoke w/ dwitte about this for a bit. after looking at the "logic" for mInitInfo, we decided that the logic isn't sustainable nor is it worth it. It's definitely wrong/faulty 
The following in pseudo code (no particular language) 
x=new nsWebBrowser
assert(x.mInitInfo)
x.create()
assert(x.mInitInfo ^ x.mDocShell)
x.setDocShell(0);
check([x.mInitInfo, x.mDocShell])

This patch isn't tested, it's a checkpoint

Comment 2

[timeless]

Attachment: rewrite logic

Comment 3

[timeless]

Attachment: compiling

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 293565 [details] [diff] [review]
compiling

in a lot of places, your indentation is inconsistent with the existing code... please don't check this in as it is, I'd rather not mess up the whitespace of this file.

+    nsCOMPtr<nsIWebProgressListener> wpl(static_cast<nsIWebProgressListener*>(mDocShellTreeOwner));

why make this a comptr?

+        if(!mInternalWidget)

as long as you're touching this line, add a space between if and (

@@ -1618,16 +1633,17 @@ NS_IMETHODIMP nsWebBrowser::SetDocShell(
+        ClearInitInfo();         

hm... why? (also, this line has trailing whitespace)

+struct nsWebBrowserInitInfo
 {

please add a comment in this struct that if new members get added, ClearInitInfo must be changed.

+   nsWebBrowserInitInfo       mInitInfo;

why did you move these? and into a "weak reference" section?

Comment 5

[timeless]

Attachment: whitespace cleanup (prereq)

Comment 6

[(no longer active)]

Is this code going to be removed in the near future (given that it's embedding)?

Comment 7

[Boris Zbarsky [:bzbarsky]]

No.

Comment 8

[Benjamin Smedberg]

Marking a bunch of bugs in the "Embedding: APIs" component INCOMPLETE in preparation to archive that component. If I have done this incorrectly, please reopen the bugs and move them to a more correct component as we don't have "embedding" APIs any more.