Issue 10580043: Run a single FilterEngine instance in a separate process (Closed)
|
Created:
May 16, 2013, 4:42 a.m. by Felix Dahlke Modified:
Nov. 12, 2013, 10:10 a.m. Visibility:
Public. |
DescriptionThese are all the changes we made in the ipc branch.
Note that there are several TODOs yet unaddressed. I think it's best to review and merge the branch now, so that we don't diverge further. All the new code not marked with TODO should be clean and robust.
Note that while this fixes blocking and hiding, it breaks the settings page.
@Oleksandr: I added you as a reviewer as well because I refactored quite a bit before uploading the review.
Patch Set 1 : #
Total comments: 63
Patch Set 2 : Addressed all issues #
Total comments: 9
Patch Set 3 : Addressed review issues #
MessagesTotal messages: 11
http://codereview.adblockplus.org/10580043/diff/6001/AdBlocker/AdBlocker.vcxproj File AdBlocker/AdBlocker.vcxproj (right): http://codereview.adblockplus.org/10580043/diff/6001/AdBlocker/AdBlocker.vcxp... AdBlocker/AdBlocker.vcxproj:454: <AdditionalLibraryDirectories>$(SolutionDir)..\libadblockplus\build\ia32\build\Debug\lib;%(AdditionalLibraryDirectories)</AdditionalLibraryDirectories> This change is unrelated to IPC, was necessary because of build changes in libadblockplus.
http://codereview.adblockplus.org/10580043/diff/6001/.hgignore File .hgignore (right): http://codereview.adblockplus.org/10580043/diff/6001/.hgignore#newcode30 .hgignore:30: Debug Production I hope this will be addressed while merging the branch - all build files should go to the build/ directory. http://codereview.adblockplus.org/10580043/diff/6001/AdPlugin.sln File AdPlugin.sln (right): http://codereview.adblockplus.org/10580043/diff/6001/AdPlugin.sln#newcode3 AdPlugin.sln:3: Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "AdblockPlus", "AdBlocker\AdBlocker.vcxproj", "{24AAF1A9-ACC0-4BEB-B781-084734D939B5}" Please note that the same change happened on default branch as well :) http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/Adbloc... File AdblockPlusEngine/AdblockPlusEngine.vcxproj (right): http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/Adbloc... AdblockPlusEngine/AdblockPlusEngine.vcxproj:15: </ProjectConfiguration> Shouldn't there be a Debug Test configuration as well? http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/Adbloc... AdblockPlusEngine/AdblockPlusEngine.vcxproj:87: <OutputFile>$(SolutionDir)AdBlocker\Debug Production\AdblockPlusEngine.exe</OutputFile> Change "Debug Production" into $(Configuration) here and for the other configurations so that this setting is identical everywhere? http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.cpp File AdblockPlusEngine/main.cpp (right): http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:7: #include <Sddl.h> I think that the usual order of includes is from generic towards more specific. I would order these like that: #include <iostream> #include <sstream> #include <vector> #include <Sddl.h> #include <ShlObj.h> #include <Windows.h> #include <AdblockPlus.h> In other words, default STL headers first, then Windows headers, then our generic headers and finally the header for this particular file. Mozilla's style guide doesn't seem to have a rule on that and I don't have strong feelings about that - but IMHO this order helps create overview (unlike strict alphabetic order). http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:39: static int references; Unused member? http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:65: // TODO: This is some pretty hacky marshalling, replace it with something more robust That's an understatement :) http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:84: int size = WideCharToMultiByte(CP_UTF8, 0, value.c_str(), -1, 0, 0, 0, 0); WideCharToMultiByte() will return 0 on error, this needs to be handled. Also, please pass the string size explicitly here, not -1. You might actually get a string with a null character in it... http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:85: char* converted = new char[size]; std::auto_ptr please, don't delete manually. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:87: std::string string(converted); string(converted, size) please - you have an explicit size. Please consider writing directly to std::string buffer as done in AdblockPlus::Utils::ToUTF8String(). Or maybe just using AdblockPlus::Utils::ToUTF8String(). http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:108: } while (hasError); The flow is rather hard to follow here, particularly with a hasError variable that doesn't indicate error. How about: bool done = false; while (!done) { if (ReadFile(...)) done = true; else if (GetLastError() != ERROR_MORE_DATA) throw std::runtime_error(...); stream << std::string(...); } http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:116: throw std::runtime_error("Failed to write to pipe"); What if bytesWritten < message.length()? According to documentation, this can happen when "writing to a non-blocking, byte-mode pipe handle with insufficient buffer space". I guess that our pipe handle is blocking and we can safely ignore that scenario? http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:137: WaitForSingleObject(filterEngineMutex, INFINITE); Doesn't this duplicate the functionality of mutexes we already have in libadblockplus? JS calls are already serialized. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:149: CloseHandle(pipe); Opening a new connection for each calls sounds like quite some overhead - it would make more sense to have the clients establish a connection when they initialize and keep it up indefinitely while exchanging messages back and forth. The engine would shut down then the last client connection disappears. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:154: bool IsWindowsVistaOrLater() Any reason why some functions are inside an empty namespace while others aren't? I cannot really see the logic here. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:169: if (FAILED(SHGetKnownFolderPath(FOLDERID_LocalAppDataLow, 0, 0, &dataPath))) I don't think that statically compiling against SHGetKnownFolderPath will work - that function doesn't exist on Windows XP, consequently loading the application will fail. You need to work with LoadLibrary() and GetProcAddress() here. Also, we might want to try passing KF_FLAG_CREATE - not sure it helps much however if the folder doesn't exist since we are running at low integrity. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:171: wcscpy_s(appDataPath, dataPath); Why do we need two copy operations here? Converting to std::wstring immediately without any buffers makes more sense. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:190: while (!filterEngine->IsInitialized()) TODO comment here please - we don't really want to poll the filter engine. And we don't want to make assumptions about how much time it takes to initialize. E.g. consider the scenario that there is high load on the machine or some disk I/O that completely occupied the hard disk. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:216: LOW_INTEGRITY_SDDL_SACL_W, SDDL_REVISION_1, &securitydescriptor, 0); You are leaking this security descriptor, it needs to be freed with LocalFree(). http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:225: int WINAPI WinMain(HINSTANCE, HINSTANCE, LPSTR, int) Second HINSTANCE parameter is hPrevInstance - you probably want to exit immediately if it is set (client got impatient). http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:257: } IMHO, ReadMessage(), WriteMessage(), MarshalStrings() and UnmarshalStrings() should be shared between the engine and the client - you need a source file shared by the two projects. http://codereview.adblockplus.org/10580043/diff/6001/Shared/AdblockPlusClient... File Shared/AdblockPlusClient.cpp (right): http://codereview.adblockplus.org/10580043/diff/6001/Shared/AdblockPlusClient... Shared/AdblockPlusClient.cpp:116: if (!CreateProcessAsUser(newToken, 0, const_cast<wchar_t*>(engineExecutablePath.c_str()), 0, 0, 0, 0, 0, 0, You are leaking the handles created here: "Handles in PROCESS_INFORMATION must be closed with CloseHandle when they are no longer needed." http://codereview.adblockplus.org/10580043/diff/6001/Shared/AdblockPlusClient... Shared/AdblockPlusClient.cpp:131: SpawnAdblockPlusEngine(); I can't say that I like the logic here, timing assumptions and such... We need something more in line with the example under http://msdn.microsoft.com/en-us/library/windows/desktop/aa365592%28v=vs.85%29...: * Call CreateFile() immediately - if it succeeds then we can just work with the pipe. * If we get INVALID_HANDLE_VALUE then the engine is probably not running, start it. If it's running but not initialized yet - no problem, the engine will exit if there is a previous instance. * After starting then engine we need to call WaitNamedPipe() and wait indefinitely - no assumptions about how long it takes to initialize the engine. We can occasionally call GetExitCodeProcess() for the engine process - if it returned with an error code then we probably don't want to wait any longer. http://codereview.adblockplus.org/10580043/diff/6001/Shared/AdblockPlusClient... Shared/AdblockPlusClient.cpp:288: std::string CallAdblockPlusEngineProcedure(const std::string& name, const std::vector<std::string>& args) Why combine the parameters here? It's a private helper anyway, the callers can simply create the complete vector that needs to be sent.
Agree with Wladimir's comments everywhere else. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/Adbloc... File AdblockPlusEngine/AdblockPlusEngine.vcxproj (right): http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/Adbloc... AdblockPlusEngine/AdblockPlusEngine.vcxproj:15: </ProjectConfiguration> Not necessary. Currently the Debug Test would build Debug Production version of AdblockPlusEngine. There's nothing "test-specific" in AdblockPlusEngine. On 2013/05/16 11:52:16, Wladimir Palant wrote: > Shouldn't there be a Debug Test configuration as well? http://codereview.adblockplus.org/10580043/diff/6001/Shared/AdblockPlusClient... File Shared/AdblockPlusClient.cpp (right): http://codereview.adblockplus.org/10580043/diff/6001/Shared/AdblockPlusClient... Shared/AdblockPlusClient.cpp:131: SpawnAdblockPlusEngine(); The documentation says "If no instances of the specified named pipe exist, the WaitNamedPipe function returns immediately, regardless of the time-out value." My understanding is that this is the reason why the approach you've described won't work. The pipe has to be created, before WaitNamedPipe, since that just waits for ConnectNamedPipe On 2013/05/16 11:52:16, Wladimir Palant wrote: > I can't say that I like the logic here, timing assumptions and such... > > We need something more in line with the example under > http://msdn.microsoft.com/en-us/library/windows/desktop/aa365592%2528v=vs.85%...: > > * Call CreateFile() immediately - if it succeeds then we can just work with the > pipe. > * If we get INVALID_HANDLE_VALUE then the engine is probably not running, start > it. If it's running but not initialized yet - no problem, the engine will exit > if there is a previous instance. > * After starting then engine we need to call WaitNamedPipe() and wait > indefinitely - no assumptions about how long it takes to initialize the engine. > We can occasionally call GetExitCodeProcess() for the engine process - if it > returned with an error code then we probably don't want to wait any longer.
http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/Adbloc... File AdblockPlusEngine/AdblockPlusEngine.vcxproj (right): http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/Adbloc... AdblockPlusEngine/AdblockPlusEngine.vcxproj:15: </ProjectConfiguration> On 2013/05/16 13:23:22, Oleksandr wrote: > Not necessary. Currently the Debug Test would build Debug Production version of > AdblockPlusEngine. There's nothing "test-specific" in AdblockPlusEngine. Then there shouldn't be a Release Test configuration either...
Some comments below. I'll address all the other issues. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.cpp File AdblockPlusEngine/main.cpp (right): http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:7: #include <Sddl.h> On 2013/05/16 11:52:16, Wladimir Palant wrote: > I think that the usual order of includes is from generic towards more specific. > I would order these like that: > > #include <iostream> > #include <sstream> > #include <vector> > > #include <Sddl.h> > #include <ShlObj.h> > #include <Windows.h> > > #include <AdblockPlus.h> > > In other words, default STL headers first, then Windows headers, then our > generic headers and finally the header for this particular file. > > Mozilla's style guide doesn't seem to have a rule on that and I don't have > strong feelings about that - but IMHO this order helps create overview (unlike > strict alphabetic order). We're using alphabetical in most places, but it's not consistent across files. I've ordered headers the way you prefer it for a few years, always ended up being messy over time, so I moved to alphabetical at some point. I think we should either address this across all files or just let it be a bit inconsistent, which I don't mind. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:87: std::string string(converted); On 2013/05/16 11:52:16, Wladimir Palant wrote: > string(converted, size) please - you have an explicit size. Please consider > writing directly to std::string buffer as done in > AdblockPlus::Utils::ToUTF8String(). Or maybe just using > AdblockPlus::Utils::ToUTF8String(). Writing directly to the string buffer is still not something I'd recommend. In C++11, strings are guaranteed to be stored continuously. In earlier standards, they are not, it's just that most compilers happen to do do it. The additional copy doesn't hurt us here. But that just as an aside, I'll guess we'll use ToUTF8String(). http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:116: throw std::runtime_error("Failed to write to pipe"); On 2013/05/16 11:52:16, Wladimir Palant wrote: > What if bytesWritten < message.length()? According to documentation, this can > happen when "writing to a non-blocking, byte-mode pipe handle with insufficient > buffer space". I guess that our pipe handle is blocking and we can safely ignore > that scenario? Yes, it's blocking. And writing long strings works (e.g. element hiding rules). We still have to pass bytesWritten in unfortunately. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:149: CloseHandle(pipe); On 2013/05/16 11:52:16, Wladimir Palant wrote: > Opening a new connection for each calls sounds like quite some overhead - it > would make more sense to have the clients establish a connection when they > initialize and keep it up indefinitely while exchanging messages back and forth. > The engine would shut down then the last client connection disappears. I previously read that there is a limit to the number of named pipe instances. However, dug a bit deeper and it appears to be unlimited. So yeah, we'll keep them open. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:154: bool IsWindowsVistaOrLater() On 2013/05/16 11:52:16, Wladimir Palant wrote: > Any reason why some functions are inside an empty namespace while others aren't? > I cannot really see the logic here. Forgot to move it there. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:171: wcscpy_s(appDataPath, dataPath); On 2013/05/16 11:52:16, Wladimir Palant wrote: > Why do we need two copy operations here? Converting to std::wstring immediately > without any buffers makes more sense. I think that's because SHGetKnownFolderPath wants a pointer to a pointer, setting it to memory it allocates for the path. Further below, however, we assume it's on the stack. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:190: while (!filterEngine->IsInitialized()) On 2013/05/16 11:52:16, Wladimir Palant wrote: > TODO comment here please - we don't really want to poll the filter engine. And > we don't want to make assumptions about how much time it takes to initialize. > E.g. consider the scenario that there is high load on the machine or some disk > I/O that completely occupied the hard disk. I presume that's not necessary anymore, now that you apparently made the FilterEngine block until it's initialised? (http://codereview.adblockplus.org/10420020/) http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:257: } On 2013/05/16 11:52:16, Wladimir Palant wrote: > IMHO, ReadMessage(), WriteMessage(), MarshalStrings() and UnmarshalStrings() > should be shared between the engine and the client - you need a source file > shared by the two projects. Same here, there's a TODO for that :) http://codereview.adblockplus.org/10580043/diff/6001/Shared/AdblockPlusClient... File Shared/AdblockPlusClient.cpp (right): http://codereview.adblockplus.org/10580043/diff/6001/Shared/AdblockPlusClient... Shared/AdblockPlusClient.cpp:131: SpawnAdblockPlusEngine(); On 2013/05/16 13:23:22, Oleksandr wrote: > The documentation says "If no instances of the specified named pipe exist, the > WaitNamedPipe function returns immediately, regardless of the time-out value." > > My understanding is that this is the reason why the approach you've described > won't work. The pipe has to be created, before WaitNamedPipe, since that just > waits for ConnectNamedPipe > > On 2013/05/16 11:52:16, Wladimir Palant wrote: > > I can't say that I like the logic here, timing assumptions and such... > > > > We need something more in line with the example under > > > http://msdn.microsoft.com/en-us/library/windows/desktop/aa365592%252528v=vs.8...: > > > > * Call CreateFile() immediately - if it succeeds then we can just work with > the > > pipe. > > * If we get INVALID_HANDLE_VALUE then the engine is probably not running, > start > > it. If it's running but not initialized yet - no problem, the engine will exit > > if there is a previous instance. > > * After starting then engine we need to call WaitNamedPipe() and wait > > indefinitely - no assumptions about how long it takes to initialize the > engine. > > We can occasionally call GetExitCodeProcess() for the engine process - if it > > returned with an error code then we probably don't want to wait any longer. > Yes, Oleksandr is right. We are using WaitNamedPipe when opening it, but here we need to wait a while for the spawned engine to create the pipe.
http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.cpp File AdblockPlusEngine/main.cpp (right): http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:171: wcscpy_s(appDataPath, dataPath); Then maybe get rid of that assumption? :) Declare std::wstring appDataPath above and do appDataPath.assign(dataPath) here. And move the wchar_t buffer into the else branch since we only need it there. Please note that SHGetKnownFolderPath() no longer guarantees that the path will fit into our buffer - it can be longer than MAX_PATH. So better not assume that it will. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:190: while (!filterEngine->IsInitialized()) Not pushed yet but - yes, once that review is done we can get rid of the polling here. http://codereview.adblockplus.org/10580043/diff/6001/Shared/AdblockPlusClient... File Shared/AdblockPlusClient.cpp (right): http://codereview.adblockplus.org/10580043/diff/6001/Shared/AdblockPlusClient... Shared/AdblockPlusClient.cpp:131: SpawnAdblockPlusEngine(); Ok, I wonder whether we can wait for pipe creation somehow... And we probably want the engine to create it sooner but only start waiting for connections once everything is initialized.
Uploaded a new patch set addressing all issues. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/Adbloc... File AdblockPlusEngine/AdblockPlusEngine.vcxproj (right): http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/Adbloc... AdblockPlusEngine/AdblockPlusEngine.vcxproj:15: </ProjectConfiguration> On 2013/05/17 05:23:51, Wladimir Palant wrote: > On 2013/05/16 13:23:22, Oleksandr wrote: > > Not necessary. Currently the Debug Test would build Debug Production version > of > > AdblockPlusEngine. There's nothing "test-specific" in AdblockPlusEngine. > > Then there shouldn't be a Release Test configuration either... Done, removed ReleaseTest. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/Adbloc... AdblockPlusEngine/AdblockPlusEngine.vcxproj:87: <OutputFile>$(SolutionDir)AdBlocker\Debug Production\AdblockPlusEngine.exe</OutputFile> On 2013/05/16 11:52:16, Wladimir Palant wrote: > Change "Debug Production" into $(Configuration) here and for the other > configurations so that this setting is identical everywhere? Done. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.cpp File AdblockPlusEngine/main.cpp (right): http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:39: static int references; On 2013/05/16 11:52:16, Wladimir Palant wrote: > Unused member? Done. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:84: int size = WideCharToMultiByte(CP_UTF8, 0, value.c_str(), -1, 0, 0, 0, 0); On 2013/05/16 11:52:16, Wladimir Palant wrote: > WideCharToMultiByte() will return 0 on error, this needs to be handled. Also, > please pass the string size explicitly here, not -1. You might actually get a > string with a null character in it... Done. Learnt something here: I thought std::string::length() would just count the bytes until the first binary 0 like strlen, but it does in fact return the full length of strings with 0 in the middle. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:85: char* converted = new char[size]; On 2013/05/16 11:52:16, Wladimir Palant wrote: > std::auto_ptr please, don't delete manually. Done. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:87: std::string string(converted); On 2013/05/17 09:11:36, Felix H. Dahlke wrote: > On 2013/05/16 11:52:16, Wladimir Palant wrote: > > string(converted, size) please - you have an explicit size. Please consider > > writing directly to std::string buffer as done in > > AdblockPlus::Utils::ToUTF8String(). Or maybe just using > > AdblockPlus::Utils::ToUTF8String(). > > Writing directly to the string buffer is still not something I'd recommend. In > C++11, strings are guaranteed to be stored continuously. In earlier standards, > they are not, it's just that most compilers happen to do do it. The additional > copy doesn't hurt us here. > > But that just as an aside, I'll guess we'll use ToUTF8String(). Decided to keep the function after all. ToUTF8String is an internal utility of libadblockplus, not really part of it's API. Wouldn't want to expose it just to make things easier for us here. Objections? http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:108: } while (hasError); On 2013/05/16 11:52:16, Wladimir Palant wrote: > The flow is rather hard to follow here, particularly with a hasError variable > that doesn't indicate error. How about: > > bool done = false; > while (!done) > { > if (ReadFile(...)) > done = true; > else if (GetLastError() != ERROR_MORE_DATA) > throw std::runtime_error(...); > > stream << std::string(...); > } Done. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:137: WaitForSingleObject(filterEngineMutex, INFINITE); On 2013/05/16 11:52:16, Wladimir Palant wrote: > Doesn't this duplicate the functionality of mutexes we already have in > libadblockplus? JS calls are already serialized. Done. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:149: CloseHandle(pipe); On 2013/05/17 09:11:36, Felix H. Dahlke wrote: > On 2013/05/16 11:52:16, Wladimir Palant wrote: > > Opening a new connection for each calls sounds like quite some overhead - it > > would make more sense to have the clients establish a connection when they > > initialize and keep it up indefinitely while exchanging messages back and > forth. > > The engine would shut down then the last client connection disappears. > > I previously read that there is a limit to the number of named pipe instances. > However, dug a bit deeper and it appears to be unlimited. So yeah, we'll keep > them open. Began to work on this, but it's a bit tricky and I'd like to do it after the merge to not hold things back. Added a TODO. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:169: if (FAILED(SHGetKnownFolderPath(FOLDERID_LocalAppDataLow, 0, 0, &dataPath))) On 2013/05/16 11:52:16, Wladimir Palant wrote: > I don't think that statically compiling against SHGetKnownFolderPath will work - > that function doesn't exist on Windows XP, consequently loading the application > will fail. You need to work with LoadLibrary() and GetProcAddress() here. > > Also, we might want to try passing KF_FLAG_CREATE - not sure it helps much > however if the folder doesn't exist since we are running at low integrity. I'll leave this to Oleksandr, since he's working on getting the plugin to work on XP. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:171: wcscpy_s(appDataPath, dataPath); On 2013/05/17 10:35:02, Wladimir Palant wrote: > Then maybe get rid of that assumption? :) > > Declare std::wstring appDataPath above and do appDataPath.assign(dataPath) here. > And move the wchar_t buffer into the else branch since we only need it there. > > Please note that SHGetKnownFolderPath() no longer guarantees that the path will > fit into our buffer - it can be longer than MAX_PATH. So better not assume that > it will. Done. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:190: while (!filterEngine->IsInitialized()) On 2013/05/17 10:35:02, Wladimir Palant wrote: > Not pushed yet but - yes, once that review is done we can get rid of the polling > here. Done. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:216: LOW_INTEGRITY_SDDL_SACL_W, SDDL_REVISION_1, &securitydescriptor, 0); On 2013/05/16 11:52:16, Wladimir Palant wrote: > You are leaking this security descriptor, it needs to be freed with LocalFree(). Done. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:225: int WINAPI WinMain(HINSTANCE, HINSTANCE, LPSTR, int) On 2013/05/16 11:52:16, Wladimir Palant wrote: > Second HINSTANCE parameter is hPrevInstance - you probably want to exit > immediately if it is set (client got impatient). Seems to be always 0 in Win32: http://blogs.msdn.com/b/oldnewthing/archive/2004/06/15/156022.aspx http://codereview.adblockplus.org/10580043/diff/6001/Shared/AdblockPlusClient... File Shared/AdblockPlusClient.cpp (right): http://codereview.adblockplus.org/10580043/diff/6001/Shared/AdblockPlusClient... Shared/AdblockPlusClient.cpp:116: if (!CreateProcessAsUser(newToken, 0, const_cast<wchar_t*>(engineExecutablePath.c_str()), 0, 0, 0, 0, 0, 0, On 2013/05/16 11:52:16, Wladimir Palant wrote: > You are leaking the handles created here: "Handles in PROCESS_INFORMATION must > be closed with CloseHandle when they are no longer needed." Done.
http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.cpp File AdblockPlusEngine/main.cpp (right): http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:169: if (FAILED(SHGetKnownFolderPath(FOLDERID_LocalAppDataLow, 0, 0, &dataPath))) We are not statically compiling against SHGetKnownFolderPath. We use delay-loading of Shell32.dll (aka link it dynamically). See the project file. On 2013/05/23 12:31:20, Felix H. Dahlke wrote: > On 2013/05/16 11:52:16, Wladimir Palant wrote: > > I don't think that statically compiling against SHGetKnownFolderPath will work > - > > that function doesn't exist on Windows XP, consequently loading the > application > > will fail. You need to work with LoadLibrary() and GetProcAddress() here. > > > > Also, we might want to try passing KF_FLAG_CREATE - not sure it helps much > > however if the folder doesn't exist since we are running at low integrity. > > I'll leave this to Oleksandr, since he's working on getting the plugin to work > on XP. http://codereview.adblockplus.org/10580043/diff/18001/AdblockPlusEngine/Adblo... File AdblockPlusEngine/AdblockPlusEngine.vcxproj (right): http://codereview.adblockplus.org/10580043/diff/18001/AdblockPlusEngine/Adblo... AdblockPlusEngine/AdblockPlusEngine.vcxproj:68: <DelayLoadDLLs>Shell32.dll</DelayLoadDLLs> This is to dynamically load SHGetKnownFolderPath on Vista+
http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.cpp File AdblockPlusEngine/main.cpp (right): http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:87: std::string string(converted); No real objections but the function name should at least indicate that a charset conversion is happening here - so I would prefer ToUtf8String() and the same contents as in libadblockplus. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:225: int WINAPI WinMain(HINSTANCE, HINSTANCE, LPSTR, int) On 2013/05/23 12:31:20, Felix H. Dahlke wrote: > Seems to be always 0 in Win32: > http://blogs.msdn.com/b/oldnewthing/archive/2004/06/15/156022.aspx Right, a bit of outdated knowledge on my end :-( Finding an already running application instance is still necessary, just more complicated - named mutexes should do. Note that the name of this mutex should also be user-specific. http://codereview.adblockplus.org/10580043/diff/6001/Shared/AdblockPlusClient... File Shared/AdblockPlusClient.cpp (right): http://codereview.adblockplus.org/10580043/diff/6001/Shared/AdblockPlusClient... Shared/AdblockPlusClient.cpp:288: std::string CallAdblockPlusEngineProcedure(const std::string& name, const std::vector<std::string>& args) On 2013/05/16 11:52:16, Wladimir Palant wrote: > Why combine the parameters here? It's a private helper anyway, the callers can > simply create the complete vector that needs to be sent. This comment doesn't seem to be addressed. http://codereview.adblockplus.org/10580043/diff/18001/AdblockPlusEngine/main.cpp File AdblockPlusEngine/main.cpp (right): http://codereview.adblockplus.org/10580043/diff/18001/AdblockPlusEngine/main.... AdblockPlusEngine/main.cpp:11: const std::wstring pipeName = L"\\\\.\\pipe\\adblockplusengine"; I realized that the pipe name should be user-specific - if a different user runs Adblock Plus on the same machine, this user should have a different pipe. Please use GetUserName() and incorporate it into the pipe name. http://codereview.adblockplus.org/10580043/diff/18001/AdblockPlusEngine/main.... AdblockPlusEngine/main.cpp:82: int size = WideCharToMultiByte(CP_UTF8, 0, value.c_str(), value.length(), 0, 0, 0, 0); Error case (size is 0 for a value with non-zero length) still isn't handled here. http://codereview.adblockplus.org/10580043/diff/18001/AdblockPlusEngine/main.... AdblockPlusEngine/main.cpp:178: return std::wstring(appDataPath) + L"\\AdblockPlus"; appDataPath already is a wstring, no need to construct a new string here.
Uploaded a new patch set addressing all issues, except those discussed below. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.cpp File AdblockPlusEngine/main.cpp (right): http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:87: std::string string(converted); On 2013/05/23 14:25:01, Wladimir Palant wrote: > No real objections but the function name should at least indicate that a charset > conversion is happening here - so I would prefer ToUtf8String() and the same > contents as in libadblockplus. Just copied the function from libadblockplus, although I think it does a few unnecessary things and I'm not a fan of writing to the internal buffer of an std::string. But nothing I can't live with. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:225: int WINAPI WinMain(HINSTANCE, HINSTANCE, LPSTR, int) On 2013/05/23 14:25:01, Wladimir Palant wrote: > On 2013/05/23 12:31:20, Felix H. Dahlke wrote: > > Seems to be always 0 in Win32: > > http://blogs.msdn.com/b/oldnewthing/archive/2004/06/15/156022.aspx > > Right, a bit of outdated knowledge on my end :-( > Finding an already running application instance is still necessary, just more > complicated - named mutexes should do. Note that the name of this mutex should > also be user-specific. How about we just try to create the pipe initially and return immediately if it already exists? We'll want to do the CreatePipe earlier anyway I think. http://codereview.adblockplus.org/10580043/diff/6001/Shared/AdblockPlusClient... File Shared/AdblockPlusClient.cpp (right): http://codereview.adblockplus.org/10580043/diff/6001/Shared/AdblockPlusClient... Shared/AdblockPlusClient.cpp:288: std::string CallAdblockPlusEngineProcedure(const std::string& name, const std::vector<std::string>& args) On 2013/05/23 14:25:01, Wladimir Palant wrote: > On 2013/05/16 11:52:16, Wladimir Palant wrote: > > Why combine the parameters here? It's a private helper anyway, the callers can > > simply create the complete vector that needs to be sent. > > This comment doesn't seem to be addressed. Oops, seems I missed this. Addressed now, you're right it's not necessary anymore (used to marshal things a bit differently before). http://codereview.adblockplus.org/10580043/diff/18001/AdblockPlusEngine/main.cpp File AdblockPlusEngine/main.cpp (right): http://codereview.adblockplus.org/10580043/diff/18001/AdblockPlusEngine/main.... AdblockPlusEngine/main.cpp:11: const std::wstring pipeName = L"\\\\.\\pipe\\adblockplusengine"; On 2013/05/23 14:25:01, Wladimir Palant wrote: > I realized that the pipe name should be user-specific - if a different user runs > Adblock Plus on the same machine, this user should have a different pipe. Please > use GetUserName() and incorporate it into the pipe name. Oh, you're right. Appended the user name. Considered using the SID instead, but it's a hassle to get and I don't think using the name will cause problems here. http://codereview.adblockplus.org/10580043/diff/18001/AdblockPlusEngine/main.... AdblockPlusEngine/main.cpp:82: int size = WideCharToMultiByte(CP_UTF8, 0, value.c_str(), value.length(), 0, 0, 0, 0); On 2013/05/23 14:25:01, Wladimir Palant wrote: > Error case (size is 0 for a value with non-zero length) still isn't handled > here. It is, it returns an empty string. I thought it was better not to crash hard if some conversion fails. The code I copied from libadblockplus handles the return value of the first call, but not for the second, not exactly sure why. http://codereview.adblockplus.org/10580043/diff/18001/AdblockPlusEngine/main.... AdblockPlusEngine/main.cpp:178: return std::wstring(appDataPath) + L"\\AdblockPlus"; On 2013/05/23 14:25:01, Wladimir Palant wrote: > appDataPath already is a wstring, no need to construct a new string here. Done.
LGTM but please add a TODO comment on creating the pipe earlier. http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.cpp File AdblockPlusEngine/main.cpp (right): http://codereview.adblockplus.org/10580043/diff/6001/AdblockPlusEngine/main.c... AdblockPlusEngine/main.cpp:225: int WINAPI WinMain(HINSTANCE, HINSTANCE, LPSTR, int) Yes, that's something I thought about as well. This should still make sure there aren't any race conditions. http://codereview.adblockplus.org/10580043/diff/18001/AdblockPlusEngine/main.cpp File AdblockPlusEngine/main.cpp (right): http://codereview.adblockplus.org/10580043/diff/18001/AdblockPlusEngine/main.... AdblockPlusEngine/main.cpp:11: const std::wstring pipeName = L"\\\\.\\pipe\\adblockplusengine"; Not sure whether the user name can be changed but definitely not when the user is logged in - so not an issue. http://codereview.adblockplus.org/10580043/diff/18001/AdblockPlusEngine/main.... AdblockPlusEngine/main.cpp:82: int size = WideCharToMultiByte(CP_UTF8, 0, value.c_str(), value.length(), 0, 0, 0, 0); We should actually crash early here. While I cannot imagine what can cause a conversion to UTF8 to fail, I am certain that I don't want to program to try continue working after it - it might cause unexplainable issues further on. The second return value is ignored because WideCharToMultiByte supposedly checked the string on first call - it shouldn't detect any new issues on next round. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
