Issue 4937147073167360: Issue 1672 - Subscribe and listen COM events using ATL::IDispEventImpl and BEGIN_SINK_MAP (Closed)
Description# Wait for resolution reagarding http://codereview.adblockplus.org/6032593782833152/
- fix call of CPluginTabBase::OnDocumentComplete from OnDocumentComplete to pass the browser of the current frame, not the browser of the root page
- remove interface IIEPlugin from idl file because we don't need it
- remove CPluginClass::Invoke
- add SINK_MAP to CPluginClass
- remove CPluginClass::GetConnectionPoint because now we use ATL::IDispEventImpl::{DispEventAdvise,DispEventUnadvise} which do it.
- remove CPluginClass::m_nConnectionID because now it lives in ATL::IDispEventImpl
- extract the code from CPluginClass::Invoke into new methods
- change to calling convention and adjust arguments of methods which are slots for the events to stdcall because it's required by ATL::IDispEventImpl
- add more argument checks for all new and modified methods
Patch Set 1 #
Total comments: 8
Patch Set 2 : fix-NITs #
Total comments: 19
Patch Set 3 : fix accoring to comments without entry point #
Total comments: 2
Patch Set 4 : add exception wrappers #
Total comments: 4
Patch Set 5 : rebase, parent = 5d5eb4df1bf2 #
MessagesTotal messages: 22
If I understand our requirements correctly, we need to be able to run under the Windows Runtime. Unfortunately that means using this ATL framework function won't work. http://codereview.adblockplus.org/4937147073167360/diff/5629499534213120/src/... File src/plugin/PluginClass.h (right): http://codereview.adblockplus.org/4937147073167360/diff/5629499534213120/src/... src/plugin/PluginClass.h:35: public ATL::IDispEventImpl<1, CPluginClass, &DIID_DWebBrowserEvents2, &LIBID_SHDocVw, 1, 1> From http://msdn.microsoft.com/en-us/library/d7eecxta.aspx "This class and its members cannot be used in applications that execute in the Windows Runtime."
Looks great, with just a few nits. Looks like there will be some merge conflicts at this point. Can you please update this to the tip? http://codereview.adblockplus.org/4937147073167360/diff/5629499534213120/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/4937147073167360/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:249: m_isAdviced = true; NIT: m_isAdvised. I know it was my spelling mistake to begin with, but now seems like a good time to fix it. We can also change Unadvice to Unadvise as well, since we are already touching it. http://codereview.adblockplus.org/4937147073167360/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:496: std::wstring url(V_BSTR(urlVariant), SysStringLen(V_BSTR(urlVariant))); I would prefer minimizing the amount of macros we use. How about urlVariant.bstrVal? http://codereview.adblockplus.org/4937147073167360/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:592: void STDMETHODCALLTYPE CPluginClass::OnCommandStateChange(long /*command*/, VARIANT_BOOL /*enable*/) Is there any reason why parameter names are commented out? http://codereview.adblockplus.org/4937147073167360/diff/5629499534213120/src/... File src/plugin/PluginClass.h (right): http://codereview.adblockplus.org/4937147073167360/diff/5629499534213120/src/... src/plugin/PluginClass.h:35: public ATL::IDispEventImpl<1, CPluginClass, &DIID_DWebBrowserEvents2, &LIBID_SHDocVw, 1, 1> On 2014/12/31 17:03:15, Eric wrote: > From http://msdn.microsoft.com/en-us/library/d7eecxta.aspx > "This class and its members cannot be used in applications that execute in > the Windows Runtime." For reference: discussion on this moved to the comments section in the issue
rebased and fixed http://codereview.adblockplus.org/4937147073167360/diff/5629499534213120/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/4937147073167360/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:249: m_isAdviced = true; On 2015/01/09 00:02:17, Oleksandr wrote: > NIT: m_isAdvised. I know it was my spelling mistake to begin with, but now seems > like a good time to fix it. We can also change Unadvice to Unadvise as well, > since we are already touching it. renamed http://codereview.adblockplus.org/4937147073167360/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:496: std::wstring url(V_BSTR(urlVariant), SysStringLen(V_BSTR(urlVariant))); On 2015/01/09 00:02:17, Oleksandr wrote: > I would prefer minimizing the amount of macros we use. How about > urlVariant.bstrVal? Sure, fine for me, fixed. http://codereview.adblockplus.org/4937147073167360/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:592: void STDMETHODCALLTYPE CPluginClass::OnCommandStateChange(long /*command*/, VARIANT_BOOL /*enable*/) On 2015/01/09 00:02:17, Oleksandr wrote: > Is there any reason why parameter names are commented out? I comment them to avoid warning that the parameter is not used.
http://codereview.adblockplus.org/4937147073167360/diff/5766466041282560/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/4937147073167360/diff/5766466041282560/src/... src/plugin/PluginClass.cpp:309: // Unadvise I guess we can loose the captain obvious comment here. http://codereview.adblockplus.org/4937147073167360/diff/5766466041282560/src/... src/plugin/PluginClass.cpp:584: } This doesn't seem to be rebased to the current tip. (rev. 746)
See #1173 about the need to deal with entry points correctly. https://issues.adblockplus.org/ticket/1173 http://codereview.adblockplus.org/4937147073167360/diff/5766466041282560/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/4937147073167360/diff/5766466041282560/src/... src/plugin/PluginClass.cpp:246: HRESULT hr = DispEventAdvise(webBrowser); The Advise/Unadvise life cycle belong in a constructor/destructor pair. Right now setting up the advice relationship happens in loose code here and in a separate function elsewhere. It would be clearer to localize these to the same place in the code. http://codereview.adblockplus.org/4937147073167360/diff/5766466041282560/src/... src/plugin/PluginClass.cpp:477: // Entry point We need to annotate all the entry points with a consistent textual marker for auditing. Unfortunately the function signature isn't consistent. http://codereview.adblockplus.org/4937147073167360/diff/5766466041282560/src/... src/plugin/PluginClass.cpp:486: { This patch set changes OnBeforeNavigate2 into an entry point. That means it needs a 'try' statement and a catch-all block. http://codereview.adblockplus.org/4937147073167360/diff/5766466041282560/src/... src/plugin/PluginClass.cpp:532: // Entry point http://codereview.adblockplus.org/4937147073167360/diff/5766466041282560/src/... src/plugin/PluginClass.cpp:537: // Entry point http://codereview.adblockplus.org/4937147073167360/diff/5766466041282560/src/... src/plugin/PluginClass.cpp:539: { try-statement and catch-all http://codereview.adblockplus.org/4937147073167360/diff/5766466041282560/src/... src/plugin/PluginClass.cpp:547: // Entry point http://codereview.adblockplus.org/4937147073167360/diff/5766466041282560/src/... src/plugin/PluginClass.cpp:549: { try-statement and catch-all http://codereview.adblockplus.org/4937147073167360/diff/5766466041282560/src/... src/plugin/PluginClass.cpp:566: // Entry point http://codereview.adblockplus.org/4937147073167360/diff/5766466041282560/src/... src/plugin/PluginClass.cpp:568: { try-statement and catch-all http://codereview.adblockplus.org/4937147073167360/diff/5766466041282560/src/... src/plugin/PluginClass.cpp:592: // Entry point http://codereview.adblockplus.org/4937147073167360/diff/5766466041282560/src/... src/plugin/PluginClass.cpp:594: { try-statement and catch-all http://codereview.adblockplus.org/4937147073167360/diff/5766466041282560/src/... src/plugin/PluginClass.cpp:1602: void STDMETHODCALLTYPE CPluginClass::Unadvise() If no OnQuit function is provided, this is an entry point. http://codereview.adblockplus.org/4937147073167360/diff/5766466041282560/src/... File src/plugin/PluginClass.h (right): http://codereview.adblockplus.org/4937147073167360/diff/5766466041282560/src/... src/plugin/PluginClass.h:61: SINK_ENTRY_EX(1, DIID_DWebBrowserEvents2, DISPID_ONQUIT, Unadvise) I'd prefer to see an OnQuit function explicitly, even if its initial version only contains a single line.
I've not addressed try-catch and entry points here, because I've created a proposal how it can be in a more flexible way. I think it would be better to merge it separately because it does not affect any change in the existing code logic so it's easier to review it and the commit would be cleaner. Unfortunately rietveld cannot detect some cases and complicates them but one can try to apply the patch and use KDiff3 for example. http://codereview.adblockplus.org/6032593782833152/ http://codereview.adblockplus.org/4937147073167360/diff/5766466041282560/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/4937147073167360/diff/5766466041282560/src/... src/plugin/PluginClass.cpp:246: HRESULT hr = DispEventAdvise(webBrowser); On 2015/01/12 14:18:07, Eric wrote: > The Advise/Unadvise life cycle belong in a constructor/destructor pair. It does not belong to constructor/destructor pair. > > Right now setting up the advice relationship happens in loose code here and in a > separate function elsewhere. It would be clearer to localize these to the same > place in the code. I see, it definitely makes sense to have a pair Advise/Unadvise which works with m_isAdvised and makes necessary calls. But I would like to do it as another issue may be even GFB. We have the method `Unadvise` to avoid code duplication because it's called from several places but "advising" is called only from one place here, so it should be fine to have it as is from the simplification point of view. As well as it's always better to have smaller commits. http://codereview.adblockplus.org/4937147073167360/diff/5766466041282560/src/... src/plugin/PluginClass.cpp:309: // Unadvise On 2015/01/10 23:27:24, Oleksandr wrote: > I guess we can loose the captain obvious comment here. removed
On 2015/01/13 11:49:12, sergei wrote: > I've not addressed try-catch and entry points here, because I've created a > proposal how it can be in a more flexible way. We don't need flexibility first. What we need first is simple catch-all block to prevent the plugin from crashing when we throw an exception. Flexibility or whatever else we decide upon after we have some experience with this can happen later, but it absolutely should not block making immediate progress on some very basic exception safety. > I think it would be better to merge it separately because it does not affect any > change in the existing code logic so it's easier to review it and the commit > would be cleaner. This commit is already extensive enough that adding what is mostly indentation does not seriously complicate what's already here.
http://codereview.adblockplus.org/4937147073167360/diff/5766466041282560/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/4937147073167360/diff/5766466041282560/src/... src/plugin/PluginClass.cpp:246: HRESULT hr = DispEventAdvise(webBrowser); On 2015/01/13 11:49:12, sergei wrote: > It does not belong to constructor/destructor pair. It absolutely should be paired. As it's written now, there's a race condition between calling DispEventAdvise() and setting 'm_isAdvised'. In what should be the destructor, there's a semaphore-protected block that ensures that DispEventUnadvise() is called atomically with resetting 'm_isAdvised' (which is defective--see comment--but whatever). There should be a similar semaphore-protected block for DispEventAdvise(). So this pair of calls is managing a kind of event-connection resource, and because it's a resource, it falls under the purview of RAII. Unlike with connection points, there's no explicit handle or object associated with the resource, but it's still a resource. The member 'm_isAdvised' acts as the proxy for that resource. Such a resource class should have two members: -- a browser pointer, to avoid calling the (de facto) global GetBrowser() in the destructor. -- the semaphore used to protect the block. Per #1467, we can use the standard library for this. The class does not need 'm_isAdvised', because it would be redundant. Such a member would always be true during the existence of the instance. The member variable in CPluginClass can be unique_ptr<>, which will automatically call the destructor. Presence of the allocated instance represents presence of the resource. http://codereview.adblockplus.org/4937147073167360/diff/5643440998055936/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/4937147073167360/diff/5643440998055936/src/... src/plugin/PluginClass.cpp:1613: m_isAdvised = false; If the call to DispEventUnadvise() fails, we should not reset the flag.
I've added try-catch. Regarding details of advising and unadvising, in general, it's not a part of this issue, if you think that we need it, please create an issue for it. However, I would like to comment that now I don't see any race condition between the call of DispEventAdvise() and setting m_isAdvised because there is no any other thread at this moment which can access it, so we don't need to protect it now. As well as, just in case, I've checked all cases when we access it. - m_isAdvised is -- set in the constructor (PluginClass.cpp), there is no any other thread at this moment which can access it, so we don't need to protect it now. -- modified and DispEventAdvise is called from CPluginClass::SetSite. At this moment there is no any other thread which can use them, so it's not necessary to protect them there. - CPluginClass::Unadvise is called from (pay attention that it contains the protection, so here we don't need to make any changes) -- another thread (CPluginClass::StartInitObject) -- "main" thread from --- exception handler (`catch (std::runtime_error e)` in CPluginClass::SetSite) --- CPluginClass::SetSite during the uninitialization --- CPluginClass::OnOnQuit Also, I would not say that we should pay attention that something can happen between DispEventAdvise() and `m_isAdvised = true;` because we have a plenty of such places, I would say it's completely enough safe here to don't care about it. > Such a resource class should have two members: > -- a browser pointer, to avoid calling the (de facto) global GetBrowser() in > the destructor. > -- the semaphore used to protect the block. Per #1467, we can use the standard > library for this. > > The class does not need 'm_isAdvised', because it would be redundant. Such a > member would always be true during the existence of the instance. > > The member variable in CPluginClass can be unique_ptr<>, which will > automatically call the destructor. Presence of the allocated instance represents > presence of the resource. In that case the semaphore cannot be a member of the resource because the semaphore will be also deinitialized which will create a race condition (e.g., another thread can be working with it during the destroying of it). However, I agree that we should not use one semaphore for the everything. Now it's even shared among all instances of CPluginClass. Also, I think we need to check whether we need `m_isAdvised` but I would say it's not important now. http://codereview.adblockplus.org/4937147073167360/diff/5643440998055936/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/4937147073167360/diff/5643440998055936/src/... src/plugin/PluginClass.cpp:1613: m_isAdvised = false; On 2015/01/13 18:44:30, Eric wrote: > If the call to DispEventUnadvise() fails, we should not reset the flag. If it's already unadvised the value of `FAILED(hr)` is true and we should set `m_isAdvised` to false. I would say it requires additional investigation whether we even need `m_isAdvised`. So, it's also not relevant to the current changes.
On 2015/02/27 18:27:36, sergei wrote: > - m_isAdvised is > -- modified and DispEventAdvise is called from CPluginClass::SetSite. At this > moment there is no any other thread which can use them, so it's not necessary to > protect them there. It is a bad idea to assert that SetSite is called only when there's no other thread running. Such a claim relies on knowing the behavior of internal threading and object pools of IE for all past and future versions. Thus, we should assume that there's a race condition here. > - CPluginClass::Unadvise is called from (pay attention that it contains the > protection, so here we don't need to make any changes) Unadvise() is implemented as an atomic, so I agree, that one is fine. At root, what I'm asking for is a parallel function Advise() which is also atomic. Now I also think that such a pair best belongs in a constructor/destructor pair. But getting to a matched pair of atomic operations is more important than having it arranged as RAII. > In that case the semaphore cannot be a member of the resource because the > semaphore will be also deinitialized which will create a race condition Such a semaphore would be a static member, as all the other semaphores we have are, such as 's_criticalSectionLocal' used in Unadvise(). > I would say it requires additional investigation whether > we even need `m_isAdvised`. So, it's also not relevant to the current changes. With better internal object architecture, we wouldn't need it. 'CPluginClass' is a horribly overburdened class, combining BHO functions and UI functions. Its initialization is also overburdened, mixing initialization for global-scope and window-scope variables. Its deferred initialization behavior isn't correct, because we are accepting events for handling before we know that the initialization thread has finished, incurring race conditions. (There's even a comment that we should wait for the initialization to finish in the destructor, which The right architecture needs a class that handles events and a class that manages the event-handling class. The manager needs to manage the advice relationship. At startup, it must first construct the handler and then call Advise(). At shutdown, it must call Unadvise(), then wait for all the events that might be in the middle of processing to complete, and only then can it destroy the handler. Right now all this is being done in 'CPluginClass', and it's a mess. Fixing the mess means fixing 'SetSite' as well, and that's not going to happen quickly. -------- In the spirit of incremental progress, it's not worth to try to fix even very many of these problems in the present change set. However, I would like to see here an atomic 'Advise()'. It's a modest start and points in the right direction. With that change, I'll be OK with leaving everything else for the future and I'll consider this change set good enough to commit.
Editing problem. Here's the complete sentence: > (There's even a comment that we should wait for the initialization to finish in the destructor, which ... is accurate only insofar as it goes, but is misleading that waiting in the destructor is all that's need to make this initialization correct.)
On 2015/02/28 19:18:22, Eric wrote: > On 2015/02/27 18:27:36, sergei wrote: > > - m_isAdvised is > > -- modified and DispEventAdvise is called from CPluginClass::SetSite. At this > > moment there is no any other thread which can use them, so it's not necessary > to > > protect them there. > > It is a bad idea to assert that SetSite is called only when there's no other > thread running. Such a claim relies on knowing the behavior of internal > threading and object pools of IE for all past and future versions. Thus, we > should assume that there's a race condition here. SetSite is only one entry point, how can there be some another thread accessing this data? > > - CPluginClass::Unadvise is called from (pay attention that it contains the > > protection, so here we don't need to make any changes) > > Unadvise() is implemented as an atomic, so I agree, that one is fine. At root, > what I'm asking for is a parallel function Advise() which is also atomic. > > Now I also think that such a pair best belongs in a constructor/destructor pair. > But getting to a matched pair of atomic operations is more important than having > it arranged as RAII. I don't mind, although it's not necessary now and it's not a part of this issue. > > In that case the semaphore cannot be a member of the resource because the > > semaphore will be also deinitialized which will create a race condition > > Such a semaphore would be a static member, as all the other semaphores we have > are, such as 's_criticalSectionLocal' used in Unadvise(). Why should it be a static memeber? It only confuses and complicates the things. > ... > In the spirit of incremental progress, it's not worth to try to fix even very > many of these problems in the present change set. However, I would like to see > here an atomic 'Advise()'. It's a modest start and points in the right > direction. With that change, I'll be OK with leaving everything else for the > future and I'll consider this change set good enough to commit. I think we don't need it because there is no case when it's needed but having it encourages the developer to assume that it's needed which complicates the understanding of the code.
On 2015/03/09 10:33:26, sergei wrote: > SetSite is only one entry point, how can there be some another thread accessing > this data? Even with a single entry point, you can get two calls to a single entry point from two different threads. Just because there's only one interface doesn't mean you can't get a race condition. The problem wouldn't be much between 'SetSite' with two non-null pointers, but with one call with a non-null pointer and another with nullptr. More generally, the design pattern that works is that if you protect one operation with a semaphore, you protect all the others. This one is no exception. --- On the other hand, since I wrote my comments originally, I've done some investigations to believe that the semaphore we have isn't doing anything useful. The apparent behavior is that (IE-internal) browser objects instantiate a single BHO object as a private member and use it exclusively, and that no browser object is active in more than one thread at a time. If this is true, then we would never need a semaphore. Yet we don't really know if this is always the behavior. And even if it's the behavior now it might not be the behavior in the future. The right thing to do is to assume that 'CPluginClass' is not used in any particular pattern. That means, in the long run, behaving correctly in general circumstances. --- Apropos the present change set, I'm willing to pass on fixing this for the moment. The problems are more severe than just a missing protection block. The present change set doesn't seem to introduce new difficulties, even if it doesn't take the opportunity to fix any of them, either. I'll approve this, simply because we need to move forward, even if it's not as far forward as I think we should be. We'll fix these defects in a future work. With all these caveats, LGTM.
> The apparent behavior is that (IE-internal) browser objects instantiate a single > BHO object as a private member and use it exclusively, and that no browser object > is active in more than one thread at a time. In theory it is possible that IE calls our IObjectWithSite implementation in a non-thread safe manner, mainly because our installer is registering a ThreadingModel="Both" and our CPluginClass is derrived from CComObjectRootEx<CComMultiThreadModel>. In fact the documentations says IE might do just that: https://msdn.microsoft.com/en-us/library/aa753617%28v=vs.85%29.aspx#wsbe_Thre.... However if we ever have problems with this (and I really doubt we ever will) I think we can just change the threading model. In any case I don't think it should be subject of this changeset.
LGTM
http://codereview.adblockplus.org/4937147073167360/diff/5631943370604544/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/4937147073167360/diff/5631943370604544/src/... src/plugin/PluginClass.cpp:491: /* [in] */ IDispatch* frameBrowserDisp, Since we do have Doxygen set up, how about using /**< [in] */ instead of /* [in] */
Noticed something else. Not a blocker to commit. http://codereview.adblockplus.org/4937147073167360/diff/5631943370604544/src/... File src/plugin/PluginClass.cpp (left): http://codereview.adblockplus.org/4937147073167360/diff/5631943370604544/src/... src/plugin/PluginClass.cpp:630: DEBUG_GENERAL(tmp); FYI. Some of the individual event-handling functions do not have a debug trace that corresponds this statement. I don't find this anything like a fatal flaw, but it's worth noting.
Please take a look at it again, because I've rebased it.
Patch set 5 LGTM
http://codereview.adblockplus.org/4937147073167360/diff/5631943370604544/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/4937147073167360/diff/5631943370604544/src/... src/plugin/PluginClass.cpp:491: /* [in] */ IDispatch* frameBrowserDisp, On 2015/03/12 05:10:57, Oleksandr wrote: > Since we do have Doxygen set up, how about using > /**< [in] */ instead of /* [in] */ So how about this comment?
http://codereview.adblockplus.org/4937147073167360/diff/5631943370604544/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/4937147073167360/diff/5631943370604544/src/... src/plugin/PluginClass.cpp:491: /* [in] */ IDispatch* frameBrowserDisp, On 2015/03/13 16:42:03, Oleksandr wrote: > On 2015/03/12 05:10:57, Oleksandr wrote: > > Since we do have Doxygen set up, how about using > > /**< [in] */ instead of /* [in] */ > > So how about this comment? I've tested it, neither /* [in] */ nor /**< [in] */ are recognized by doxygen (1.8.9.1). Actually, I could remove it, should I remove them?
This was pushed here https://hg.adblockplus.org/adblockplusie/rev/66c71b83b105 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
