309232 - [FIX]nsWebBrowser::AddWebBrowserListener always returns NS_ERROR_INVALID_ARG if called before window is created

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect, P3)

Description

[Geoff Schmidt]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6
Build Identifier: GRE release 1.7.8 (unchanged in CVS)

In nsWebBrowser::AddWebBrowserListener in nswebbrowser.cpp, rv is initialized to
NS_ERROR_INVALID_ARG. Then there are two branches of control, after which rv is
returned to the caller. One branch handles the case where the window has already
been created; it sets rv to the return value of BindListener. The other branch
handles the case where the window hasn't been created. It queues the listener
object until window creation. However, it does not set rv, so even if everything
goes fine NS_ERROR_INVALID_ARG is returned.

If this is the correct behavior (it is supposed to be an error to add a listener
before calling InitWindow) this should be documented.

Reproducible: Always

Steps to Reproduce:
1. Instantiate NS_WEBBROWSER_CONTRACTID; get the nsIWebBrowser interface.
2. Call AddWebBrowserListener on it, passing NS_GET_IID(nsIWebProgressListener)
and a weak reference to an object that implements nsIWebProgressListener.
Actual Results:  
AddWebBrowserListener returns NS_ERROR_INVALID_ARG. Haven't checked to see if
notifications are delivered anyway.

Expected Results:  
Expected return value is NS_OK.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: This should fix it

Comment 2

[Darin Fisher]

Comment on attachment 207796 [details] [diff] [review]
This should fix it

Perhaps it would be better to use nsAutoPtr<nsWebBrowserListenerState> to avoid having to insert all of those NS_DELETEXPCOM(state) calls.  You could just do a .forget() if AppendElement succeeds.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to review comments

Comment 4

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 208019 [details] [diff] [review]
Updated to review comments

I think it's worth taking this on the 1.8 branch.  This just makes us not throw in a non-error case in our embedding API...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Fixed on branch.