308245 - nsWebBrowser::Create fails to handle docShell(do_CreateInstance("@mozilla.org/docshell;1")) failing [@ nsWebBrowser::BindListener]

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[timeless]

this could happen in oom (it happened to me because of something else, but...)

docShell.mRawPtr = 0x0
webbrwsr!nsWebBrowser::Create(void)+0x1d9 (FPO: [Non-Fpo]) (CONV: stdcall)
[r:\mozilla\embedding\browser\webbrowser\nswebbrowser.cpp @ 1145]
winEmbed!WebBrowserChrome::CreateBrowser(int aX = -1, int aY = -1, int aCX = -1,
int aCY = -1, class nsIWebBrowser ** aBrowser = 0x0012fe00)+0x160 (FPO:
[Non-Fpo]) (CONV: thiscall)
[r:\mozilla\embedding\tests\winembed\webbrowserchrome.cpp @ 91]
winEmbed!AppCallbacks::CreateBrowserWindow(unsigned int aChromeFlags = 0xffe,
class nsIWebBrowserChrome * aParent = 0x00000000, class nsIWebBrowserChrome **
aNewWindow = 0x0012fedc)+0xaf (FPO: [Non-Fpo]) (CONV: cdecl)
[r:\mozilla\embedding\tests\winembed\winembed.cpp @ 1150]
winEmbed!OpenWebPage(char * url = 0x0040b068
"http://www.mozilla.org/projects/minimo")+0x35 (FPO: [Non-Fpo]) (CONV: cdecl)
[r:\mozilla\embedding\tests\winembed\winembed.cpp @ 231]
winEmbed!main(int argc = 1, char ** argv = 0x00358728)+0x174 (FPO: [Non-Fpo])
(CONV: cdecl) [r:\mozilla\embedding\tests\winembed\winembed.cpp @ 175]
winEmbed!mainCRTStartup(void)+0x12c (FPO: [Non-Fpo]) (CONV: cdecl)
[f:\vs70builds\3077\vc\crtbld\crt\src\crtexe.c @ 398]
kernel32!BaseProcessStart+0x23 (FPO: [Non-Fpo])

(2bb4.44f0): Access violation - code c0000005 (first chance)
webbrwsr!nsWebBrowser::BindListener+0x88:
01a62658 8b02             mov     eax,[edx]         ds:0023:00000000=????????
webbrwsr!nsWebBrowser::BindListener(class nsISupports * aListener = 0x00f6d6ac,
struct nsID * aIID = 0x01a7291c)+0x88 (FPO: [Non-Fpo]) (CONV: stdcall)
[r:\mozilla\embedding\browser\webbrowser\nswebbrowser.cpp @ 275]
webbrwsr!nsWebBrowser::Create(void)+0x3e0 (FPO: [Non-Fpo]) (CONV: stdcall)
[r:\mozilla\embedding\browser\webbrowser\nswebbrowser.cpp @ 1184]
winEmbed!WebBrowserChrome::CreateBrowser(int aX = -1, int aY = -1, int aCX = -1,
int aCY = -1, class nsIWebBrowser ** aBrowser = 0x0012fe00)+0x160 (FPO:
[Non-Fpo]) (CONV: thiscall)

Comment 1

[Chris Lawson (gone)]

See also bug 431454. I wonder if maybe this is the same crash. timeless, any ideas? If you think it is, dupe this over there, since we have reliable STR and a compreg.dat file that can trigger it.

Comment 2

[timeless]

you're almost certainly hitting the something else, iirc bad compreg could explain that.

just post the obvious patch.

Comment 3

[timeless]

Attachment: non minimal patch (whitespace changes, logic swaps)

i hate docshell.

Comment 4

[timeless]

Attachment: for review

Comment 5

[Boris Zbarsky [:bzbarsky]]

I likely won't be able to review this until sometime in June.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So how come this is reindenting the world?  Can we please not do that?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 318565 [details] [diff] [review]
for review

I'd like to see a patch that doesn't reindent the world.

Comment 8

[timeless]

Attachment: less for review

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 372601 [details] [diff] [review]
less for review

>Index: nsWebBrowser.cpp
>       nsCOMPtr<nsILookAndFeel> laf = do_GetService(kLookAndFeelCID);
>+        if (laf)
>       laf->GetColor(nsILookAndFeel::eColor_WindowBackground, mBackgroundColor);

Fix the indent on your new line here (and on the line that's now in the if body).  And if !laf, set background to a sane color, instead of leaving it random?  Or is it already set to a sane color?

With that, r=me.

Comment 10

[timeless]

mBackgroundColor is initialized by the constructor to 0 (black?) i haven't changed that. I could change it to something else if you like....

to answer your question about why i was reindenting the world, it uses 3 space indentation and 3-4 space indentation. Sometimes it uses 2 space indentation....

Usually it uses 4-4 space indentation which is what I was trying to match.

Do you want this pushed as is with the one indentation fixed as you indicated?

I still would like somehow to fix the rest of the indentation in this file as it's a real mess.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> mBackgroundColor is initialized by the constructor to 0 (black?)

That's fine.

> to answer your question about why i was reindenting the world

It wasn't a question.  It was a "I want a patch that doesn't reindent the world".  If you want to do the reindent, I'd recommend a separate bug (and a different reviewer from me, probably)....

Comment 12

[timeless]

Attachment: merged

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 383778 [details] [diff] [review]
merged

>       nsCOMPtr<nsILookAndFeel> laf = do_GetService(kLookAndFeelCID);
>+        if (laf)
>       laf->GetColor(nsILookAndFeel::eColor_WindowBackground, mBackgroundColor);

>@@ -1827,6 +1829,9 @@ NS_IMETHODIMP nsWebBrowser::OpenStream(n
> 
>   if (!mStream) {
>     mStream = new nsEmbedStream();
>+        if (!mStream)
>+             return NS_ERROR_OUT_OF_MEMORY;

Nits: there are still at least these bad indentations.

Comment 15

[timeless]

http://hg.mozilla.org/mozilla-central/rev/cfdb650315aa