Issue 29377825: Issue 4951 - Restrict request headers in XMLHttpRequest.Also test Accept-Encoding with th… (Closed)
|
Created:
March 2, 2017, 9:16 p.m. by hub Modified:
March 16, 2017, 5:12 p.m. Base URL:
https://hg.adblockplus.org/libadblockplus/ Visibility:
Public. |
DescriptionIssue 4951 - Restrict request headers in XMLHttpRequest.
Also test Accept-Encoding with the XHR support.
Patch Set 1 : Initial changes #
Total comments: 14
Patch Set 2 : Reworked the testing. Addressed review comments. #
Total comments: 4
Patch Set 3 : improve tests following feedback. #
Total comments: 9
Patch Set 4 : updated patch with feedback #Patch Set 5 : Forgot one more change from the review feedback. #Patch Set 6 : Shorten the "sleep" is the wait loop. #
Total comments: 2
Patch Set 7 : Updated based on the feedback. #
Total comments: 20
MessagesTotal messages: 30
https://codereview.adblockplus.org/29377825/diff/29377829/lib/compat.js File lib/compat.js (right): https://codereview.adblockplus.org/29377825/diff/29377829/lib/compat.js#newco... lib/compat.js:327: if (this._forbiddenRequestHeaders[header.toLowerCase()] !== undefined) { Actually it's not according to our coding style, so I would recommend to use hasOwnProperty. https://codereview.adblockplus.org/29377825/diff/29377829/lib/compat.js#newco... lib/compat.js:422: console.warning("Attempt to set a forbidden header was denied: " + name); the name of the method should be "warn" https://codereview.adblockplus.org/29377825/diff/29377829/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29377829/test/WebRequest.cpp... test/WebRequest.cpp:171: // The test will override console.warning so that the I think we should rather check in WebRequest::GET that there are no forbidden headers and other headers are indeed present. In addition I think we should leave the check of the warnings and although I have some concerns about overriding of console.warn here instead of using LogSystem I think it's fine in tests. https://codereview.adblockplus.org/29377825/diff/29377829/test/WebRequest.cpp... test/WebRequest.cpp:173: // While this is an implementation detail, since the DOM API Could you please remove "DOM" because it's not a DOM API. https://codereview.adblockplus.org/29377825/diff/29377829/test/WebRequest.cpp... test/WebRequest.cpp:182: // test 'Accept-Encoding' is rejected What about having several tests, at least checking forbidden headers and any other headers? BTW, comments look very good for test name, although unfortunately we are not using this approach. https://codereview.adblockplus.org/29377825/diff/29377829/test/WebRequest.cpp... test/WebRequest.cpp:194: value = value = jsEngine->Evaluate("result"); value = value = https://codereview.adblockplus.org/29377825/diff/29377829/test/WebRequest.cpp... test/WebRequest.cpp:195: ASSERT_TRUE(value->IsUndefined()); I think we should use EXPECT_* when the test can continue the execution.
BTW, what about a new issue for it?
https://codereview.adblockplus.org/29377825/diff/29377829/lib/compat.js File lib/compat.js (right): https://codereview.adblockplus.org/29377825/diff/29377829/lib/compat.js#newco... lib/compat.js:327: if (this._forbiddenRequestHeaders[header.toLowerCase()] !== undefined) { On 2017/03/02 22:25:31, sergei wrote: > Actually it's not according to our coding style, so I would recommend to use > hasOwnProperty. Acknowledged. https://codereview.adblockplus.org/29377825/diff/29377829/lib/compat.js#newco... lib/compat.js:422: console.warning("Attempt to set a forbidden header was denied: " + name); On 2017/03/02 22:25:31, sergei wrote: > the name of the method should be "warn" Looks like my testing has failed here. Will definitely fix that. https://codereview.adblockplus.org/29377825/diff/29377829/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29377829/test/WebRequest.cpp... test/WebRequest.cpp:171: // The test will override console.warning so that the On 2017/03/02 22:25:31, sergei wrote: > I think we should rather check in WebRequest::GET that there are no forbidden > headers and other headers are indeed present. > > In addition I think we should leave the check of the warnings and although I > have some concerns about overriding of console.warn here instead of using > LogSystem I think it's fine in tests. I didn't realize there was LogSystem to check for that. And given the other mistake (console.warning instead of console.warn), I would never have realized. Will definitely fix that. https://codereview.adblockplus.org/29377825/diff/29377829/test/WebRequest.cpp... test/WebRequest.cpp:173: // While this is an implementation detail, since the DOM API On 2017/03/02 22:25:31, sergei wrote: > Could you please remove "DOM" because it's not a DOM API. Acknowledged. https://codereview.adblockplus.org/29377825/diff/29377829/test/WebRequest.cpp... test/WebRequest.cpp:194: value = value = jsEngine->Evaluate("result"); On 2017/03/02 22:25:31, sergei wrote: > value = value = Acknowledged. Cut&paste error. I missed it. https://codereview.adblockplus.org/29377825/diff/29377829/test/WebRequest.cpp... test/WebRequest.cpp:195: ASSERT_TRUE(value->IsUndefined()); On 2017/03/02 22:25:31, sergei wrote: > I think we should use EXPECT_* when the test can continue the execution. Acknowledged.
Looks pretty good, I just have a few minor things. Plus, I agree with Sergei that a separate issue would probably be best for this rather than doing it as part of 4916, mind creating one? https://codereview.adblockplus.org/29377825/diff/29377832/lib/compat.js File lib/compat.js (right): https://codereview.adblockplus.org/29377825/diff/29377832/lib/compat.js#newco... lib/compat.js:302: "accept-charset": true, The v8 version we use should support Set [1], it seems that'd be nicer to use here. But I don't feel strong about that. [1]: https://developer.mozilla.org/de/docs/Web/JavaScript/Reference/Global_Objects... https://codereview.adblockplus.org/29377825/diff/29377832/lib/compat.js#newco... lib/compat.js:327: if (this._forbiddenRequestHeaders.hasOwnProperty(header.toLowerCase())) { Nit: Opening braces go on their own line where possible, see: https://adblockplus.org/coding-style (Omitting the braces for single line bodies is also fine.)
https://codereview.adblockplus.org/29377825/diff/29377829/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29377829/test/WebRequest.cpp... test/WebRequest.cpp:171: // The test will override console.warning so that the What about check in WebRequest::GET?
https://codereview.adblockplus.org/29377825/diff/29377832/lib/compat.js File lib/compat.js (right): https://codereview.adblockplus.org/29377825/diff/29377832/lib/compat.js#newco... lib/compat.js:302: "accept-charset": true, On 2017/03/03 08:33:05, Felix Dahlke wrote: > The v8 version we use should support Set [1], it seems that'd be nicer to use > here. But I don't feel strong about that. > > [1]: > https://developer.mozilla.org/de/docs/Web/JavaScript/Reference/Global_Objects... It seems not C++ exception with description "ReferenceError: Set is not defined at compat.js:301" thrown in SetUp(). :-( https://codereview.adblockplus.org/29377825/diff/29377832/lib/compat.js#newco... lib/compat.js:327: if (this._forbiddenRequestHeaders.hasOwnProperty(header.toLowerCase())) { On 2017/03/03 08:33:05, Felix Dahlke wrote: > Nit: Opening braces go on their own line where possible, see: > https://adblockplus.org/coding-style > > (Omitting the braces for single line bodies is also fine.) Acknowledged.
On 2017/03/03 08:33:05, Felix Dahlke wrote: > Looks pretty good, I just have a few minor things. Plus, I agree with Sergei > that a separate issue would probably be best for this rather than doing it as > part of 4916, mind creating one? Filed https://issues.adblockplus.org/ticket/4951 Will update the commit message, etc.
On 2017/03/03 08:38:59, sergei wrote: > What about check in WebRequest::GET? This new patch does that now. Thanks
https://codereview.adblockplus.org/29377825/diff/29377868/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29377868/test/WebRequest.cpp... test/WebRequest.cpp:40: class XHRTestWebRequest : public AdblockPlus::WebRequest IMHO it'd make more sense to add lastRequestHeaders to MockWebRequest rather than to add a new mock here. https://codereview.adblockplus.org/29377825/diff/29377868/test/WebRequest.cpp... test/WebRequest.cpp:218: ResetTestXHR(const AdblockPlus::JsEnginePtr & jsEngine, const CatchLogSystemPtr & logger) Nit: `AdblockPlus::JsEnginePtr &` -> `AdblockPlus::JsEnginePtr&` (same "snuggling" as with pointers). https://codereview.adblockplus.org/29377825/diff/29377868/test/WebRequest.cpp... test/WebRequest.cpp:237: } while (jsEngine->Evaluate("result")->IsUndefined()) This is a bit of a footgun, wouldn't it work to just wait 200 ms after each invocation as we do in the other tests? Since no actual request is happening that shouldn't be shaky I believe.
will update the patch https://codereview.adblockplus.org/29377825/diff/29377868/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29377868/test/WebRequest.cpp... test/WebRequest.cpp:40: class XHRTestWebRequest : public AdblockPlus::WebRequest On 2017/03/06 16:51:07, Felix Dahlke wrote: > IMHO it'd make more sense to add lastRequestHeaders to MockWebRequest rather > than to add a new mock here. I can do that too. I was trying to avoid side effect my isolating test fixture, even though I don't see any obvious one right now. https://codereview.adblockplus.org/29377825/diff/29377868/test/WebRequest.cpp... test/WebRequest.cpp:218: ResetTestXHR(const AdblockPlus::JsEnginePtr & jsEngine, const CatchLogSystemPtr & logger) On 2017/03/06 16:51:07, Felix Dahlke wrote: > Nit: `AdblockPlus::JsEnginePtr &` -> `AdblockPlus::JsEnginePtr&` (same > "snuggling" as with pointers). Acknowledged. https://codereview.adblockplus.org/29377825/diff/29377868/test/WebRequest.cpp... test/WebRequest.cpp:237: } while (jsEngine->Evaluate("result")->IsUndefined()) On 2017/03/06 16:51:08, Felix Dahlke wrote: > This is a bit of a footgun, wouldn't it work to just wait 200 ms after each > invocation as we do in the other tests? Since no actual request is happening > that shouldn't be shaky I believe. The only difference I see is that we'll check after 200ms rather than 60. Also the difference is that MockWebRequest has a Sleep(50) in its GET method. But then when I implement the change above, it will be there, so I can just change the sleep to 200.... I was trying to avoid having the tests drag for too long needlessly. The while() loop is here to change on a regular basis.
https://codereview.adblockplus.org/29377825/diff/29377868/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29377868/test/WebRequest.cpp... test/WebRequest.cpp:237: } while (jsEngine->Evaluate("result")->IsUndefined()) On 2017/03/06 17:26:05, hub wrote: > On 2017/03/06 16:51:08, Felix Dahlke wrote: > > This is a bit of a footgun, wouldn't it work to just wait 200 ms after each > > invocation as we do in the other tests? Since no actual request is happening > > that shouldn't be shaky I believe. > > The only difference I see is that we'll check after 200ms rather than 60. Also > the difference is that MockWebRequest has a Sleep(50) in its GET method. But > then when I implement the change above, it will be there, so I can just change > the sleep to 200.... > > I was trying to avoid having the tests drag for too long needlessly. The while() > loop is here to change on a regular basis. Oh sorry, it seems I didn't get what I meant here across :) I surely see no point in sleeping here for longer. Could probably be shorter than 60 ms the way the code works now, since it's checking for `result`. What I meant was: In the other tests we do not have logic for checking that the XHR is actually finished. We just wait for 200 ms and that always seems to do the trick. So what I suggested was: Get rid of `WAIT_FOR_XHR_RESULT` and replace it with a single `AdblockPlus::Sleep(200)` call, which is how the other tests seem to do it. I can see the reasoning for not wanting to sleep an arbitrary amount of time and hoping for the best, but I don't see (I might be missing something though) how the problem is different in this test than it is in the others. (What I meant by footgun: If someone calls `WAIT_FOR_XHR_RESULT` without first calling `request.send()`, this will just hang indefinitely.)
https://codereview.adblockplus.org/29377825/diff/29377868/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29377868/test/WebRequest.cpp... test/WebRequest.cpp:237: } while (jsEngine->Evaluate("result")->IsUndefined()) On 2017/03/07 07:44:22, Felix Dahlke wrote: > Oh sorry, it seems I didn't get what I meant here across :) I surely see no > point in sleeping here for longer. Could probably be shorter than 60 ms the way > the code works now, since it's checking for `result`. > > What I meant was: In the other tests we do not have logic for checking that the > XHR is actually finished. We just wait for 200 ms and that always seems to do > the trick. So what I suggested was: Get rid of `WAIT_FOR_XHR_RESULT` and replace > it with a single `AdblockPlus::Sleep(200)` call, which is how the other tests > seem to do it. > > I can see the reasoning for not wanting to sleep an arbitrary amount of time and > hoping for the best, but I don't see (I might be missing something though) how > the problem is different in this test than it is in the others. The other test have a similar loop. That's where I took it. see lines: 100, 129, 146, 169, etc. WAIT_FOR_XHR_RESULT is just to shorten typing. Not to do it differently. A simple wait is IMHO doomed to fail. Either by just being too long or being too short some of the time and fail. > (What I meant by footgun: If someone calls `WAIT_FOR_XHR_RESULT` without first > calling `request.send()`, this will just hang indefinitely.) Like any other programming error. Like when I actually forget to call send() and had errors... I'm not worried about that. If we are worried for indefinite hang we can set a global timeout and abort the test with a fail in that case. But that's a different issue.
https://codereview.adblockplus.org/29377825/diff/29377868/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29377868/test/WebRequest.cpp... test/WebRequest.cpp:237: } while (jsEngine->Evaluate("result")->IsUndefined()) On 2017/03/07 15:52:32, hub wrote: > On 2017/03/07 07:44:22, Felix Dahlke wrote: > > > Oh sorry, it seems I didn't get what I meant here across :) I surely see no > > point in sleeping here for longer. Could probably be shorter than 60 ms the > way > > the code works now, since it's checking for `result`. > > > > What I meant was: In the other tests we do not have logic for checking that > the > > XHR is actually finished. We just wait for 200 ms and that always seems to do > > the trick. So what I suggested was: Get rid of `WAIT_FOR_XHR_RESULT` and > replace > > it with a single `AdblockPlus::Sleep(200)` call, which is how the other tests > > seem to do it. > > > > I can see the reasoning for not wanting to sleep an arbitrary amount of time > and > > hoping for the best, but I don't see (I might be missing something though) how > > the problem is different in this test than it is in the others. > > The other test have a similar loop. That's where I took it. > see lines: 100, 129, 146, 169, etc. > > WAIT_FOR_XHR_RESULT is just to shorten typing. Not to do it differently. A > simple wait is IMHO doomed to fail. Either by just being too long or being too > short some of the time and fail. Oh indeed, for some reason I missed that. Like I said, rusty :) Given how we have that code in a few other test cases, how about we add a function such as `WaitForVariable` (e.g. `WaitForVariable("result")`) or something to that effect? > > (What I meant by footgun: If someone calls `WAIT_FOR_XHR_RESULT` without first > > calling `request.send()`, this will just hang indefinitely.) > > Like any other programming error. Like when I actually forget to call send() and > had errors... > I'm not worried about that. > > If we are worried for indefinite hang we can set a global timeout and abort the > test with a fail in that case. But that's a different issue. I guess we're fine, we can solve that issue when it occurs. https://codereview.adblockplus.org/29377825/diff/29378669/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29378669/test/WebRequest.cpp... test/WebRequest.cpp:209: ResetTestXHR(const AdblockPlus::JsEnginePtr& jsEngine, const CatchLogSystemPtr& logger) I just noticed that we could use this function in the XMLHttpRequest test above as well. We'd have to move the `logger->clear()` call out, but that'd be the only thing.
https://codereview.adblockplus.org/29377825/diff/29378669/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29378669/test/WebRequest.cpp... test/WebRequest.cpp:209: ResetTestXHR(const AdblockPlus::JsEnginePtr& jsEngine, const CatchLogSystemPtr& logger) On 2017/03/07 17:03:43, Felix Dahlke wrote: > I just noticed that we could use this function in the XMLHttpRequest test above > as well. We'd have to move the `logger->clear()` call out, but that'd be the > only thing. Acknowledged.
maybe I forgot to comment that I have updated the patch for yesterday for the last bits of feedback.
LGTM with just one nit :) Moving Sergei to CC since he's not available right now. https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp... test/WebRequest.cpp:69: void Nit: Why the new line? Not really consistent with the rest of the code.
https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp... test/WebRequest.cpp:69: void On 2017/03/09 15:10:58, Felix Dahlke wrote: > Nit: Why the new line? Not really consistent with the rest of the code. probably habit. I'll remove it when I send you the patch for checkin.
Although it's already pushed, just in case, I would like to leave some comments. At least it can be helpful when we stumble upon some issue. https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp... test/WebRequest.cpp:29: lastRequestHeaders.clear(); Sorry, I forgot to say it again when my comments had been removed. I think we should use some unique URL in test requests to distinguish them from another requests sent from FilterEngine. May be we don't observe it on practice currently but there is a race condition because the instance of MockWebRequest is shared by all requests. BTW, there is also a data race on lastRequestHeaders because the access to it is not synchronized. https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp... test/WebRequest.cpp:69: void On 2017/03/09 15:33:02, hub wrote: > On 2017/03/09 15:10:58, Felix Dahlke wrote: > > Nit: Why the new line? Not really consistent with the rest of the code. > > probably habit. I'll remove it when I send you the patch for checkin. Just in case for future, I would rather prefer to have the final version in the review tool. https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp... test/WebRequest.cpp:86: AdblockPlus::Sleep(60); Since we touched it here I would say that I personally don't like these sleeps and would like to get rid of them at some point. In this particular case I would rather prefer something like the following: inject a C++ function which sets a mutex and call it when either 'load' or 'error' event is fired, in the test body wait for that mutex. Instead of mutex it would be better to use std::promise but it's broken in currently used visual studio. BTW, as a positive side effect we will get rid of global result variable. https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp... test/WebRequest.cpp:145: #if defined(HAVE_CURL) It seems, it should be in commit https://github.com/adblockplus/libadblockplus/commit/8ba0148ea850a278a2a9c659... or at least in a new commit but not in this one. https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp... test/WebRequest.cpp:211: auto catchLogSystem = CatchLogSystemPtr(new CatchLogSystem); I would prefer to have std::make_shared<CatchLogSystem>() or at least CatchLogSystemPtr catchLogSystem = new CatchLogSystem(); than CatchLogSystemPtr(new CatchLogSystem);. https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp... test/WebRequest.cpp:211: auto catchLogSystem = CatchLogSystemPtr(new CatchLogSystem); Could we please always use either () or {} for constructors? BTW, {} is not fully supported by currently used Visual Studio. https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp... test/WebRequest.cpp:216: std::static_pointer_cast<MockWebRequest>(jsEngine->GetWebRequest()); Although it's fine here, I would rather prefer to have MockWebRequestPtr as a member of test fixture and avoid any unnecessary down-casting.
BTW, the codereview is not closed.
https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp... test/WebRequest.cpp:211: auto catchLogSystem = CatchLogSystemPtr(new CatchLogSystem); On 2017/03/13 09:56:56, sergei wrote: > Could we please always use either () or {} for constructors? BTW, {} is not > fully supported by currently used Visual Studio. I definitely prefer omitting these when not necessary, our (i.e. Mozilla's) coding style just leaves it up to the author AFAIK.
https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp... test/WebRequest.cpp:211: auto catchLogSystem = CatchLogSystemPtr(new CatchLogSystem); On 2017/03/13 10:05:47, Felix Dahlke wrote: > On 2017/03/13 09:56:56, sergei wrote: > > Could we please always use either () or {} for constructors? BTW, {} is not > > fully supported by currently used Visual Studio. > > I definitely prefer omitting these when not necessary, our (i.e. Mozilla's) > coding style just leaves it up to the author AFAIK. It seems Mozilla's coding style does not mention it but I'm afraid it can lead to some uninitialized plain members of objects, so I would like to always have value initialization.
https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp... test/WebRequest.cpp:211: auto catchLogSystem = CatchLogSystemPtr(new CatchLogSystem); On 2017/03/13 10:31:02, sergei wrote: > On 2017/03/13 10:05:47, Felix Dahlke wrote: > > On 2017/03/13 09:56:56, sergei wrote: > > > Could we please always use either () or {} for constructors? BTW, {} is not > > > fully supported by currently used Visual Studio. > > > > I definitely prefer omitting these when not necessary, our (i.e. Mozilla's) > > coding style just leaves it up to the author AFAIK. > It seems Mozilla's coding style does not mention it but I'm afraid it can lead > to some uninitialized plain members of objects, so I would like to always have > value initialization. We are talking a bout `new CatchLogSystem` vs `new CatchLogSystem()`? I'm a bit rusty so I may be missing something, but I don't think there's any semantic difference here. {} is of course a different story.
https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp... test/WebRequest.cpp:211: auto catchLogSystem = CatchLogSystemPtr(new CatchLogSystem); On 2017/03/13 11:05:42, Felix Dahlke wrote: > On 2017/03/13 10:31:02, sergei wrote: > > On 2017/03/13 10:05:47, Felix Dahlke wrote: > > > On 2017/03/13 09:56:56, sergei wrote: > > > > Could we please always use either () or {} for constructors? BTW, {} is > not > > > > fully supported by currently used Visual Studio. > > > > > > I definitely prefer omitting these when not necessary, our (i.e. Mozilla's) > > > coding style just leaves it up to the author AFAIK. > > It seems Mozilla's coding style does not mention it but I'm afraid it can lead > > to some uninitialized plain members of objects, so I would like to always have > > value initialization. > > We are talking a bout `new CatchLogSystem` vs `new CatchLogSystem()`? Yes. > I'm a bit > rusty so I may be missing something, but I don't think there's any semantic > difference here. {} is of course a different story. In C++98 and C++03 `new T` and `new T()` were different and the definition of default initialization has been changed between C++98, C++03 and C++11 (I cannot say anything about C++14 and C++17 but I am ready to expect it to be changed there again). However, if we always use value initialization (either `()` or `{}`) then we are definitely on the safe side.
https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp... test/WebRequest.cpp:211: auto catchLogSystem = CatchLogSystemPtr(new CatchLogSystem); On 2017/03/13 11:39:16, sergei wrote: > On 2017/03/13 11:05:42, Felix Dahlke wrote: > > On 2017/03/13 10:31:02, sergei wrote: > > > On 2017/03/13 10:05:47, Felix Dahlke wrote: > > > > On 2017/03/13 09:56:56, sergei wrote: > > > > > Could we please always use either () or {} for constructors? BTW, {} is > > not > > > > > fully supported by currently used Visual Studio. > > > > > > > > I definitely prefer omitting these when not necessary, our (i.e. > Mozilla's) > > > > coding style just leaves it up to the author AFAIK. > > > It seems Mozilla's coding style does not mention it but I'm afraid it can > lead > > > to some uninitialized plain members of objects, so I would like to always > have > > > value initialization. > > > > We are talking a bout `new CatchLogSystem` vs `new CatchLogSystem()`? > Yes. > > I'm a bit > > rusty so I may be missing something, but I don't think there's any semantic > > difference here. {} is of course a different story. > In C++98 and C++03 `new T` and `new T()` were different and the definition of > default initialization has been changed between C++98, C++03 and C++11 (I cannot > say anything about C++14 and C++17 but I am ready to expect it to be changed > there again). However, if we always use value initialization (either `()` or > `{}`) then we are definitely on the safe side. I see - rusty after all :P Looking at our existing code, it appears we have just a single occurrence without parentheses (also in the Libadblockplus tests), so we could add a rule about that. Feel free to create a patch for adblockplus.org/coding-style.
https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp... test/WebRequest.cpp:211: auto catchLogSystem = CatchLogSystemPtr(new CatchLogSystem); On 2017/03/13 11:55:06, Felix Dahlke wrote: ... > Feel free to create a patch for > adblockplus.org/coding-style. I would like to firstly upgrade compilers (in particular android NDK and Visual Studio) and then we can define a subset of new features and techniques which should be used.
Posted a new patch in a new review. https://codereview.adblockplus.org/29382567 https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp... test/WebRequest.cpp:86: AdblockPlus::Sleep(60); On 2017/03/13 09:56:56, sergei wrote: > Since we touched it here I would say that I personally don't like these sleeps > and would like to get rid of them at some point. In this particular case I would > rather prefer something like the following: > inject a C++ function which sets a mutex and call it when either 'load' or > 'error' event is fired, in the test body wait for that mutex. Instead of mutex > it would be better to use std::promise but it's broken in currently used visual > studio. BTW, as a positive side effect we will get rid of global result > variable. I'll file an issue for that. https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp... test/WebRequest.cpp:211: auto catchLogSystem = CatchLogSystemPtr(new CatchLogSystem); On 2017/03/13 11:55:06, Felix Dahlke wrote: > On 2017/03/13 11:39:16, sergei wrote: > > On 2017/03/13 11:05:42, Felix Dahlke wrote: > > > On 2017/03/13 10:31:02, sergei wrote: > > > > On 2017/03/13 10:05:47, Felix Dahlke wrote: > > > > > On 2017/03/13 09:56:56, sergei wrote: > > > > > > Could we please always use either () or {} for constructors? BTW, {} > is > > > not > > > > > > fully supported by currently used Visual Studio. > > > > > > > > > > I definitely prefer omitting these when not necessary, our (i.e. > > Mozilla's) > > > > > coding style just leaves it up to the author AFAIK. > > > > It seems Mozilla's coding style does not mention it but I'm afraid it can > > lead > > > > to some uninitialized plain members of objects, so I would like to always > > have > > > > value initialization. > > > > > > We are talking a bout `new CatchLogSystem` vs `new CatchLogSystem()`? > > Yes. > > > I'm a bit > > > rusty so I may be missing something, but I don't think there's any semantic > > > difference here. {} is of course a different story. > > In C++98 and C++03 `new T` and `new T()` were different and the definition of > > default initialization has been changed between C++98, C++03 and C++11 (I > cannot > > say anything about C++14 and C++17 but I am ready to expect it to be changed > > there again). However, if we always use value initialization (either `()` or > > `{}`) then we are definitely on the safe side. > > I see - rusty after all :P Looking at our existing code, it appears we have just > a single occurrence without parentheses (also in the Libadblockplus tests), so > we could add a rule about that. Feel free to create a patch for > adblockplus.org/coding-style. I see two occurences, line 60 and 61 of this file. https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp... test/WebRequest.cpp:211: auto catchLogSystem = CatchLogSystemPtr(new CatchLogSystem); On 2017/03/13 09:56:57, sergei wrote: > I would prefer to have std::make_shared<CatchLogSystem>() or at least > CatchLogSystemPtr catchLogSystem = new CatchLogSystem(); than > CatchLogSystemPtr(new CatchLogSystem);. I did submit a follow up patch to address these. In a new review. https://codereview.adblockplus.org/29382567
https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp... test/WebRequest.cpp:86: AdblockPlus::Sleep(60); On 2017/03/13 14:16:51, hub wrote: > I'll file an issue for that. filed https://issues.adblockplus.org/ticket/4983
https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp... test/WebRequest.cpp:29: lastRequestHeaders.clear(); On 2017/03/13 09:56:57, sergei wrote: > Sorry, I forgot to say it again when my comments had been removed. I think we > should use some unique URL in test requests to distinguish them from another > requests sent from FilterEngine. May be we don't observe it on practice > currently but there is a race condition because the instance of MockWebRequest > is shared by all requests. BTW, there is also a data race on lastRequestHeaders > because the access to it is not synchronized. What about this point? Actually I find it very important.
On 2017/03/16 11:08:25, sergei wrote: > https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp > File test/WebRequest.cpp (right): > > https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp... > test/WebRequest.cpp:29: lastRequestHeaders.clear(); > On 2017/03/13 09:56:57, sergei wrote: > > Sorry, I forgot to say it again when my comments had been removed. I think we > > should use some unique URL in test requests to distinguish them from another > > requests sent from FilterEngine. May be we don't observe it on practice > > currently but there is a race condition because the instance of MockWebRequest > > is shared by all requests. BTW, there is also a data race on > lastRequestHeaders > > because the access to it is not synchronized. > > What about this point? Actually I find it very important. Filed an issue: https://issues.adblockplus.org/ticket/5002 Will work on it. Thanks 
|
