Issue 11167069: Fix the crash. IInternetBindInfo BINDSTRING_XDR_ORIGIN bug workaround. (Closed)
|
Created:
July 24, 2013, 8:10 a.m. by Oleksandr Modified:
Aug. 14, 2013, 6:24 a.m. Visibility:
Public. |
DescriptionFix the crash. IInternetBindInfo BINDSTRING_XDR_ORIGIN bug workaround.
Patch Set 1 #
Total comments: 6
Patch Set 2 : Comment addressed #
Total comments: 1
Patch Set 3 : Removed unused hr #
Total comments: 1
Patch Set 4 : Another unused hr removed #MessagesTotal messages: 11
The issue was that there is a bug in HTML component downloader object. (.htc links). Since HTML component is used in css3pie for css behavior - we get a crash on quite a few of sites. The idea here is to detect if the request is initiated by a weird object, and don't try to retrieve a domain name if it is.
http://codereview.adblockplus.org/11167069/diff/1/src/plugin/PluginWbPassThro... File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/11167069/diff/1/src/plugin/PluginWbPassThro... src/plugin/PluginWbPassThrough.cpp:152: if ((hr == S_OK) && domainRetrieved && (resLen > 0)) Shouldn't this be moved into the if as well? What happens if resLen is not null but bindToObject was not FALSE? We'll do something with domainRetrieved which is 0.
http://codereview.adblockplus.org/11167069/diff/1/src/plugin/PluginWbPassThro... File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/11167069/diff/1/src/plugin/PluginWbPassThro... src/plugin/PluginWbPassThrough.cpp:152: if ((hr == S_OK) && domainRetrieved && (resLen > 0)) I might be missing something but we will not do anything with the domainRetrieved, because it's 0 in this if. So the end result would be an empty string boundDomain. On 2013/08/05 14:28:05, Felix H. Dahlke wrote: > Shouldn't this be moved into the if as well? What happens if resLen is not null > but bindToObject was not FALSE? We'll do something with domainRetrieved which is > 0.
http://codereview.adblockplus.org/11167069/diff/1/src/plugin/PluginWbPassThro... File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/11167069/diff/1/src/plugin/PluginWbPassThro... src/plugin/PluginWbPassThrough.cpp:140: HRESULT hr = pOIBindInfo->GetBindString(BINDSTRING_ACCEPT_MIMES, &mime, 1, &resLen); Just noticed that hr is unused here, we shouldn't set it here then and declare it on first use. http://codereview.adblockplus.org/11167069/diff/1/src/plugin/PluginWbPassThro... src/plugin/PluginWbPassThrough.cpp:152: if ((hr == S_OK) && domainRetrieved && (resLen > 0)) On 2013/08/08 14:03:20, Oleksandr wrote: > I might be missing something but we will not do anything with the > domainRetrieved, because it's 0 in this if. So the end result would be an empty > string boundDomain. > On 2013/08/05 14:28:05, Felix H. Dahlke wrote: > > Shouldn't this be moved into the if as well? What happens if resLen is not > null > > but bindToObject was not FALSE? We'll do something with domainRetrieved which > is > > 0. > Right, there's a check for domainRetrieved there, so it'll work as intended. I still think it's a bit confusing this way, we should move this code into the if above, it logically runs only under that condition.
http://codereview.adblockplus.org/11167069/diff/1/src/plugin/PluginWbPassThro... File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/11167069/diff/1/src/plugin/PluginWbPassThro... src/plugin/PluginWbPassThrough.cpp:152: if ((hr == S_OK) && domainRetrieved && (resLen > 0)) Agreed. On 2013/08/08 14:13:55, Felix H. Dahlke wrote: > On 2013/08/08 14:03:20, Oleksandr wrote: > > I might be missing something but we will not do anything with the > > domainRetrieved, because it's 0 in this if. So the end result would be an > empty > > string boundDomain. > > On 2013/08/05 14:28:05, Felix H. Dahlke wrote: > > > Shouldn't this be moved into the if as well? What happens if resLen is not > > null > > > but bindToObject was not FALSE? We'll do something with domainRetrieved > which > > is > > > 0. > > > > Right, there's a check for domainRetrieved there, so it'll work as intended. > > I still think it's a bit confusing this way, we should move this code into the > if above, it logically runs only under that condition.
http://codereview.adblockplus.org/11167069/diff/1/src/plugin/PluginWbPassThro... File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/11167069/diff/1/src/plugin/PluginWbPassThro... src/plugin/PluginWbPassThrough.cpp:140: HRESULT hr = pOIBindInfo->GetBindString(BINDSTRING_ACCEPT_MIMES, &mime, 1, &resLen); On 2013/08/08 14:13:55, Felix H. Dahlke wrote: > Just noticed that hr is unused here, we shouldn't set it here then and declare > it on first use. You haven't addressed this comment. http://codereview.adblockplus.org/11167069/diff/10001/src/plugin/PluginWbPass... File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/11167069/diff/10001/src/plugin/PluginWbPass... src/plugin/PluginWbPassThrough.cpp:146: hr = pOIBindInfo->GetBindString(BINDSTRING_FLAG_BIND_TO_OBJECT, &bindToObject, 1, &resLen); The hr we assign here is now unused.
http://codereview.adblockplus.org/11167069/diff/5003/src/plugin/PluginWbPassT... File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/11167069/diff/5003/src/plugin/PluginWbPassT... src/plugin/PluginWbPassThrough.cpp:140: HRESULT hr = pOIBindInfo->GetBindString(BINDSTRING_ACCEPT_MIMES, &mime, 1, &resLen); You still didn't address my comment here. It's gone from this change set for some reason, but it was there :D This hr is unused as well. We can declare HRESULT hr below when we call GetBindString with BINDSTRING_XDR_ORIGIN.
That's the stuff! LGTM.
LGTM 
|
