Re: [PATCH v2 1/3] mshtml: Add a null parameter check in navigate_new_window().

[Jacek Caban]

On 15.09.2017 07:34, Zebediah Figura wrote:
> v2: check in caller instead of do_query_service().
> Signed-off-by: Zebediah Figura
>
> dlls/mshtml/navigate.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/dlls/mshtml/navigate.c b/dlls/mshtml/navigate.c
> index 423d650a8c..06425c3f2b 100644
> --- a/dlls/mshtml/navigate.c
> +++ b/dlls/mshtml/navigate.c
> @@ -2246,6 +2246,12 @@ HRESULT navigate_new_window(HTMLOuterWindow *window, IUri *uri, const WCHAR *nam
>      nsChannelBSC *bsc;
>      HRESULT hres;
> - if (!window->doc_obj->client)
> - {
> -
>    FIXME("no attached client site; cannot query INewWindowManager\n");
> -
>    return E_NOTIMPL;
> - }

This doesn't look right. Look at HlinkTarget_Navigate. We explicitly
choose this code path because client is NULL. This seems to be a
regression from commit 63205bf01d6 and the right solution seems to be
simply ignoring the fact that we don't have INewWindowManager and continue.

Thanks,
Jacek

[PATCH v2 1/3] mshtml: Add a null parameter check in navigate_new_window().

[Zebediah Figura]

On 09/15/2017 07:52 AM, Jacek Caban wrote:
> On 15.09.2017 07:34, Zebediah Figura wrote:
> > v2: check in caller instead of do_query_service().
> > Signed-off-by: Zebediah Figura
> >
> > dlls/mshtml/navigate.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/dlls/mshtml/navigate.c b/dlls/mshtml/navigate.c
> > index 423d650a8c..06425c3f2b 100644
> > --- a/dlls/mshtml/navigate.c
> > +++ b/dlls/mshtml/navigate.c
> > @@ -2246,6 +2246,12 @@ HRESULT navigate_new_window(HTMLOuterWindow *window, IUri *uri, const WCHAR *nam
> >       nsChannelBSC *bsc;
> >       HRESULT hres;
> > - if (!window->doc_obj->client)
> > - {
> > -
> >    FIXME("no attached client site; cannot query INewWindowManager\n");
> > -
> >    return E_NOTIMPL;
> > - }
>
> This doesn't look right. Look at HlinkTarget_Navigate. We explicitly
> choose this code path because client is NULL. This seems to be a
> regression from commit 63205bf01d6 and the right solution seems to be
> simply ignoring the fact that we don't have INewWindowManager and continue.
>
> Thanks,
> Jacek

I see; I'll try to fix the code.