Hi Internals,
Attached are details of a suggested patch for COM extension
defect 37927 (http://bugs.php.net/bug.php?id=37927).
All comments welcome; good or bad.
Regards
Andy
Andy Wharmby
IBM United Kingdom Limited
Winchester, England SO21 2JN
E-mail: andy_wharmby@uk.ibm.com
COM Defect 37927
This defect actually reports issues with 2 different event sink
interfaces, DwebBrowserEvents2 and DWebbrowserEvents. So taking each
one in turn:
*DWebBrowserEvents2::NewWindow2 Event *
This event is documented at :
http://msdn.microsoft.com/workshop/browser/webbrowser/reference/ifaces/dwebbrowserevents2/newwindow2.asp
The defect details an issue with the NewWindow2 event handler but my
testing has shown the exact same issue with NewWindows3 event too.
The problem here is that the user defined event handler is not getting
called when the associated event occurs in IE and my investigations have
shown this
is due to a bug in the COM extension code. Examining the COM trace
output with DebugView shows:
[4716] T=00001890
[4716] PHP:IEEventSinker InvokeEx
[4716] T=00001890
[4716] -- Invoke: 251 newwindow2 [10] flags=00000001 args=2
[4716] T=00001890
[4716] alloc zval for arg 0 VT=00004009 << only one of two arguemets
gets processed !!
[4716] T=00001890
[4716] PHP:IEEventSinker InvokeEx <<next event
So the event is reported but is only partially processed by the COM
extension code; no attempt is made to call the event handler.
BTW. This COM trace in com_wrapper.c, which output using
"OutputDebugString", is active in non-DEBBUG builds unlike all other
uses in the PHP code base.
I am not sure what the cost of these calls is but I would have expected
such calls to be enabled in DEBUG builds only. Easily fixed and I will
put together
a fix unless anyone sees a need for this trace in non-DEBUG, i.e
production builds.
Anyway the reason for the partial request processing is because the code
is trapping in php_com_wrap_variant() when processing the input
arguments for the
"NewWindow2" (and NewWindow3) event. The NewWIndow2 event is defined on
MSDN as follows:
void NewWindow2(IDispatch **&ppDisp, VARIANT_BOOL *&Cancel);
i.e a double indirect pointer to a IDispatch. However, this argument is
defined for output. The VT_DISPATCH variant addressed has its type set but
its value is NULL , i.e V_DISPATCH(<var addr>) == NULL . So when
php_com_wrap_variant() attempts a call on the IDispatch interface to
get type
information and the PHP thread traps. Easily fixed by checking for a
NULL IDispatch interface pointer prior to call.
Required patch to fix is here: http://www.pastebin.ca/329136
This fixes the problem of the event handler not being called in the
first place. However any attempt by a NewWindow2/3 event handler to
cancel the navigation,
i.e by setting $cancel == TRUE, will still fail until the fix for
defect 345764 is dropped (http://bugs.php.net/bug.php?id=34564). Frank
is currently reviewing this fix.
*DWebBrowserEvents::NewWindow Event *
This event is documented at :
http://msdn.microsoft.com/workshop/browser/webbrowser/reference/ifaces/dwebbrowserevents/newwindow.asp.
However, the MSDN description does include the following warning:
"This interface is obsolete; use the DWebBrowserEvents2 interface
instead. The DWebBrowserEvents2 interface provides more
control over the WebBrowser control than the DWebBrowserEvents
interface."
From my investigations I believe this interface has fallen into a state
of disrepair and should not be used anymore. I can see no reason why
anyone would
need to use DWebBrowserEvents in preference to DWebBrowserEvents2;
given that the minimum requirements, IE and Windows levels,
are the same in both cases.
During my investigation I found 2 problems which appear IE related and
not issues in the COM extension itself:
(1) The interface defines NewWindow as an event that takes just 2
arguments. However it appears it actually now takes 6 !!.
The COM trace shows:
[7060] T=00001de4
[7060] PHP:IEEventSinker InvokeEx
[7060] T=00001de4
[7060] -- Invoke: 107 newwindow [9] flags=00000001 args=6
[7060] T=00001de4
[7060] alloc zval for arg 0 VT=00000008
[7060] T=00001de4
[7060] alloc zval for arg 1 VT=00000003
[7060] T=00001de4
[7060] alloc zval for arg 2 VT=00000008
[7060] T=00001de4
[7060] alloc zval for arg 3 VT=0000400c
[7060] T=00001de4
[7060] alloc zval for arg 4 VT=00000008
[7060] T=00001de4
[7060] alloc zval for arg 5 VT=0000400b
[7060] T=00001de4
[7060] arguments processed, prepare to do some work
[7060] T=00001de4
[7060] function called ok
So defining the event handler as described on the MSDN site will result
in unpredictable behaviour; certainly navigation will not be prevented
with the supplied testcase.
The 6th argument is of type VT_VOOL so I am guessing that's the "cancel"
argument. By adding dummy arguments to the event handler as follows:
function NewWindow(&$dum1, $dum2, $dum3, $dum4, $dum5, &$Cancel) {
$this->newWindowOpened = true;
echo "NewWindow event was fired.\n";
variant_set($Cancel,true);
return;
}
and with the fix for defect 345764 applied then the NewWindow event is
fired the first time its tried and navigation is cancelled. However any
subsequent attempt to
open any link in a new window is also prevented even though not all
attempts result in a NewWindow event being reported and occasionally I
am unable to close
down IE so perhaps my guess at what the 6th argument does is invalid!!.
Everything is fine when DWebBrowserEvents2 interface is used.
(2) The supplied testcase defines the "OnQuit" event which is, according
to MSDN, defined by the DWebBrowserEvents interface. Alas not !! Or at
least not using IE
Version 6. The actual event name appears to be "Quit" and not
"OnQuit". Again here is the COM trace of the event reported using the
supplied test case:
[6012] PHP:IEEventSinker InvokeEx
[6012] T=00000ff0
[6012] -- Invoke: 103 quit [4] flags=00000001 args=1
[6012] T=00000ff0
[6012] alloc zval for arg 0 VT=0000000b
[6012] T=00000ff0
[6012] arguments processed, prepare to do some work
[6012] T=00000ff0
[6012] failed to call func
This clearly shows the event is named "quit" and so the call to the any
handler named "onQuit" fails. Modifying the testcase to define a method
"Quit" and the user
defined method fires OK.