Issue 29319002: Issue 1737 - ABP 1.3 for IE sometimes blocks navigation to the ad from Google search results
Patch Set 1 #
Total comments: 1
MessagesTotal messages: 8
I'd like to defer this issue to after the 1.5 release. This fix looks simple enough, but I'm concerned that it's not the right way to fix it. The present fix might be flaky, depending on lucky order with race conditions (worse yet, upon an order that is almost always lucky). It's related to my least favorite single function in the whole code base, 'SetSite()'. The data structure at play here, 's_threadinstances', has entries removed in the code for 'SetSite(0)' but added elsewhere, in the event handler for 'WindowStateChanged'. This is almost certainly the wrong behavior, since 'SetSite' acts as a second-phase constructor for 'CPluginClass' (and this is somewhat necessary behavior to work as a BHO). The present code relies on the behavior that the state-change message comes in after the protocol hook. This _might_ always be the case; I'm not sure. Rather than debate this in the week before release, when we've got higher priority work to do, I'd prefer to look into this later. I've got some changes I've been working on for 'SetSite()' and 'CPluginClass' lifetime generally. I'd like to revisit this change after 1.5 is out, when we're working on these other things as well.
On 2015/06/25 19:01:32, Eric wrote: > I'd like to defer this issue to after the 1.5 release. > > This fix looks simple enough, but I'm concerned that it's not the right way to > fix it. The present fix might be flaky, depending on lucky order with race > conditions (worse yet, upon an order that is almost always lucky). > > It's related to my least favorite single function in the whole code base, > 'SetSite()'. > > The data structure at play here, 's_threadinstances', has entries removed in the > code for 'SetSite(0)' but added elsewhere, in the event handler for > 'WindowStateChanged'. This is almost certainly the wrong behavior, since > 'SetSite' acts as a second-phase constructor for 'CPluginClass' (and this is > somewhat necessary behavior to work as a BHO). The present code relies on the > behavior that the state-change message comes in after the protocol hook. This > _might_ always be the case; I'm not sure. Good point. Actually we might should add current instance into `s_threadInstances` in `SetSite` and test whether there is exactly one instance. However it won't work in case when one process is used by several tabs for example (thus one DLL instance is shared by several plugin instances). So it likely won't work when the user has a tons of opened tabs. > Rather than debate this in the week before release, when we've got higher > priority work to do, I'd prefer to look into this later. I've got some changes > I've been working on for 'SetSite()' and 'CPluginClass' lifetime generally. I'd > like to revisit this change after 1.5 is out, when we're working on these other > things as well.
https://codereview.adblockplus.org/29319002/diff/29319003/src/plugin/PluginWb... File src/plugin/PluginWbPassThrough.cpp (right): https://codereview.adblockplus.org/29319002/diff/29319003/src/plugin/PluginWb... src/plugin/PluginWbPassThrough.cpp:372: m_contentType = ContentType::CONTENT_TYPE_DOCUMENT; If we decide to accept it, actually, I think that we should not even touch `m_contentType` here rather return `nativeHr` because we are assuming that it's a root document which is never blocked. As well as JIC I would check that referrer is empty.
On 2015/06/25 19:01:32, Eric wrote: > I've been working on for 'SetSite()' and 'CPluginClass' lifetime generally. I'd > like to revisit this change after 1.5 is out, when we're working on these other > things as well. Shall we revisit this change? I assume these are the changes Eric meant: https://codereview.adblockplus.org/29323561/
On 2015/10/26 11:35:00, Oleksandr wrote: > On 2015/06/25 19:01:32, Eric wrote: > > > I've been working on for 'SetSite()' and 'CPluginClass' lifetime generally. > I'd > > like to revisit this change after 1.5 is out, when we're working on these > other > > things as well. > > Shall we revisit this change? I assume these are the changes Eric meant: > https://codereview.adblockplus.org/29323561/ That particular codereview does not affect `s_threadInstances`. Regarding the current approach, I've debugged a bit and discovered the following: It seems that fixing of the place of inserting/setting of {currentThreadId, PluginClass*} into `s_threadInstances` can be a part of the solution, the second part concerns the note about empty document URL below. First of all, we definitely should at least change the condition when to set the pair {currentThreadId, PluginClass*} in `OnWindowStateChanged`. When the new tab is created such way it's not visibled, so the tab is always nullptr within `BeginningTransaction` in such cases until the user switches the tabs. Secondly, we assume that the `BeginningTransaction` at least for the first request (important for the current issue) is called within the same thread, within which IE calls one of our methods which sets {currentThreadId, PluginClass*}. This assumption already works quite successfully when the pair {currentThreadId, PluginClass*} is set within `CPluginClass::OnWindowStateChanged`. It seems such methods like `OnWindowStateChanged`, `OnBeforeNavigate2`, etc. are called from the same thread, within which `SetSite`is called. So it seems we may move it into `SetSite()`. However, firstly, I would like to know the reason it's done exactly in `OnWindowStateChanged`. No one can guarantee that there is no any another instance of another tab in the same thread and we don't want to get the wrong tab based on the thread ID. It can be an important reason to set {currentThreadId, PluginClass*} exactly in `OnWindowStateChanged`. The note about empty document URL. Let's say we have made a decision about the place of setting the current PluginClass* for the particular thread ID. Please take a look at the sequence of the interesting calls in this case (they all are made in the same thread, IE11): - SetSite - OnBeforeNavigate2 // calls m_tab->OnNavigate(URL) which sets document URL to the correct value - OnDocumentComplete // calls m_tab->OnDocumentComplete which sets the document URL to an empty URL - OnWindowStateChanged // the tab is disbled - BeginningTransaction // here is a valid tab but the document URL is empty - OnDocumentComplete // the document URL is finally set to the correct one. I would propose to not update document URL if the new one is an empty string. The note about possible access violation. It's not related to this particular issue but it's good to know here as well, see https://issues.adblockplus.org/ticket/3234. Despite the fix can be quite trivial, I would like to make it in a separate commit.
On 2015/10/26 11:35:00, Oleksandr wrote: > Shall we revisit this change? I assume these are the changes Eric meant: > https://codereview.adblockplus.org/29323561/ TL;DR version -- Fix 's_threadInstances' first. On 2015/10/26 17:50:27, sergei wrote: > That particular codereview does not affect `s_threadInstances`. Not directly, no. It's the next cleanup of SetSite(). Fixing 's_threadInstances', though, should happen in SetSite(), so there's an implicit dependency. > It seems that fixing of the place of inserting/setting of {currentThreadId, > PluginClass*} into `s_threadInstances` can be a part of the solution It is. I've looked into this extensively already. I've already got some code for it done. (This is news. I've done more on this since I first asked it to be deferred in a comment above.) > However, firstly, I would like to know the reason it's done exactly > in `OnWindowStateChanged`. I believe it's a defect. The code for SetSite() used to be far worse than it is now, and it's still a mess. It seems that at one point it was used as a SetSite() entry point for two different CLSID. I can guess it was easier to initialize 's_threadInstances' elsewhere back then. It's not a good reason, but it may have been expedient enough at the time. > First of all, we definitely should at least change the condition when to set the > pair {currentThreadId, PluginClass*} in `OnWindowStateChanged`. The map 's_threadInstances' should be initialized in SetSite(), not where it is currently. Entries in the map are removed in SetSite(nullptr). We don't have any guarantee that any of the event handlers 'On...()' execute in the same thread. Insofar as I know, we have neither documentation nor evidence to that effect. In some cases they _are_ executed this way, but we have no guarantee of it. Luckily, we never actually use the values of the map in any of these functions, so (except for initialization) it doesn't matter what thread they execute on. As a result, at the very least we have a possible resource leak. Both adding and removing map entries use the current thread as an index. It seems possible that an entry added with one thread ID would not be removed because the erase() call happened under a different thread. The current code suppresses any error if no thread entry is found, so we'd never have seen this behavior unless we were very specifically hunting for it. I have verified that SetSite(non-null) and SetSite(nullptr) always execute in the same thread. We can add and remove entries in this function safely. Because SetSite() acts as a _de facto_ second-phase constructor and destructor, we don't have to worry that calls to find() can occur when the map is not initialized. We have a quotation from Igor Tandetnik in our code, PluginWbPassThrough.cpp:274 You are only guaranteed to be on the main STA thread in two cases. First, in methods of interfaces that were obtained with IServiceProvider, such as IHttpNegotiage::BeginningTransaction or IAuthenticate::Authenticate. I'm assuming the "main STA" thread is the same thread that SetSite() is called on. I still need to verify that before posting code. Assuming it is, the call to GetTab(DWORD) in WBPassthruSink::BeginningTransaction() will always find the correct instance. I should mention that this defect might also be the cause of https://issues.adblockplus.org/ticket/111, since one of the other places that GetTab(DWORD) is called is when setting up the status bar. > No one can guarantee that there is no any another > instance of another tab in the same thread and we don't want to get the wrong > tab based on the thread ID. From what I can tell, SetSite(non-null) is called for each tab, and each tab is called with a unique thread ID. There are also, it seems, other threads not assigned to specific tabs, and it seems always at least one. > I would propose to not update document URL if the new one is an empty string. It's possible we won't even have the problem if we initialize the thread map correctly. At the very least, let's fix it first and see what you still might need to do. > It's not related to this particular issue but it's good to know here as well, > see https://issues.adblockplus.org/ticket/3234. Despite the fix can be quite > trivial, I would like to make it in a separate commit. I agree we don't need to deal with it here.
On 2015/10/26 17:50:27, sergei wrote: > It seems such methods like > `OnWindowStateChanged`, `OnBeforeNavigate2`, etc. are called from the same > thread, within which `SetSite`is called. -- > However, firstly, I would like to know the reason it's done exactly > in `OnWindowStateChanged`. Unfortunately based on my tests a few years ago it could not guaranteed that SetSite and OnWindowStateChanged would be called from the same thread. One edge case would be to open many tabs, close IE and then reopen it so that it starts reloading all those tabs simultaneously. In that case SetSite and OnWindowStateChanged are not always synchronized. Another case is opening a tab group. (In Favorites click on the -> arrow next to the folder). So whatever we change here we'll need to make sure we don't break those edge cases. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
