Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Side by Side Diff: test/WebRequest.cpp

Issue 29408721: Issue 5002 - Fix potential concurrency issues in WebRequest tests. (Closed) Base URL: https://hg.adblockplus.org/libadblockplus/
Patch Set: Avoid casting the webRequest pointer. Created April 19, 2017, 2:40 p.m.
Left:
Right:
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
View unified diff | Download patch
« no previous file with comments | « no previous file | no next file » | no next file with comments »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
OLDNEW
1 /* 1 /*
2 * This file is part of Adblock Plus <https://adblockplus.org/>, 2 * This file is part of Adblock Plus <https://adblockplus.org/>,
3 * Copyright (C) 2006-2017 eyeo GmbH 3 * Copyright (C) 2006-2017 eyeo GmbH
4 * 4 *
5 * Adblock Plus is free software: you can redistribute it and/or modify 5 * Adblock Plus is free software: you can redistribute it and/or modify
6 * it under the terms of the GNU General Public License version 3 as 6 * it under the terms of the GNU General Public License version 3 as
7 * published by the Free Software Foundation. 7 * published by the Free Software Foundation.
8 * 8 *
9 * Adblock Plus is distributed in the hope that it will be useful, 9 * Adblock Plus is distributed in the hope that it will be useful,
10 * but WITHOUT ANY WARRANTY; without even the implied warranty of 10 * but WITHOUT ANY WARRANTY; without even the implied warranty of
(...skipping 12 matching lines...) Expand all
23 23
24 using namespace AdblockPlus; 24 using namespace AdblockPlus;
25 25
26 namespace 26 namespace
27 { 27 {
28 class MockWebRequest : public AdblockPlus::WebRequest 28 class MockWebRequest : public AdblockPlus::WebRequest
29 { 29 {
30 public: 30 public:
31 AdblockPlus::ServerResponse GET(const std::string& url, const AdblockPlus::H eaderList& requestHeaders) const 31 AdblockPlus::ServerResponse GET(const std::string& url, const AdblockPlus::H eaderList& requestHeaders) const
32 { 32 {
33 lastRequestHeaders.clear(); 33 std::set<std::string> headerNames;
34 for (auto header : requestHeaders) 34 for (auto header : requestHeaders)
35 { 35 {
36 lastRequestHeaders.insert(header.first); 36 headerNames.insert(header.first);
37 }
38 {
39 std::lock_guard<std::mutex> lock(requestHeaderNamesMutex);
40 // we currently ignore the result. We should check it actually gets inse rted.
41 requestHeaderNames.insert(std::make_pair(url, std::move(headerNames)));
37 } 42 }
38 43
39 AdblockPlus::Sleep(50); 44 AdblockPlus::Sleep(50);
40 45
41 AdblockPlus::ServerResponse result; 46 AdblockPlus::ServerResponse result;
42 result.status = NS_OK; 47 result.status = NS_OK;
43 result.responseStatus = 123; 48 result.responseStatus = 123;
44 result.responseHeaders.push_back(std::pair<std::string, std::string>("Foo" , "Bar")); 49 result.responseHeaders.push_back(std::pair<std::string, std::string>("Foo" , "Bar"));
45 result.responseText = url + "\n"; 50 result.responseText = url + "\n";
46 if (!requestHeaders.empty()) 51 if (!requestHeaders.empty())
47 { 52 {
48 result.responseText += requestHeaders[0].first + "\n" + requestHeaders[0 ].second; 53 result.responseText += requestHeaders[0].first + "\n" + requestHeaders[0 ].second;
49 } 54 }
50 return result; 55 return result;
51 } 56 }
52 57
58 // Testing method
59 // Get the headers for the request. Return a pair of a bool (found
60 // or not) and the actual header names
61 std::pair<bool, std::set<std::string>> headersForRequest(const std::string& url)
62 {
63 std::lock_guard<std::mutex> lock(requestHeaderNamesMutex);
64 auto iter = requestHeaderNames.find(url);
65 if (iter != requestHeaderNames.end())
66 {
67 auto result = std::make_pair(true, iter->second);
68 requestHeaderNames.erase(iter);
69 return result;
70 }
71 return std::make_pair(false, std::set<std::string>());
72 }
73
53 // mutable. Very Ugly. But we are testing and need to change this in GET whi ch is const. 74 // mutable. Very Ugly. But we are testing and need to change this in GET whi ch is const.
54 mutable std::set<std::string> lastRequestHeaders; 75 mutable std::mutex requestHeaderNamesMutex;
76 mutable std::map<std::string, std::set<std::string>> requestHeaderNames;
55 }; 77 };
56 78
57 template<class T> 79 template<class T>
58 class WebRequestTest : public BaseJsTest 80 class WebRequestTest : public BaseJsTest
59 { 81 {
60 protected: 82 protected:
61 void SetUp() 83 void SetUp()
62 { 84 {
63 BaseJsTest::SetUp(); 85 BaseJsTest::SetUp();
64 jsEngine->SetWebRequest(AdblockPlus::WebRequestPtr(new T())); 86 webRequest = std::shared_ptr<T>(new T());
sergei 2017/04/19 14:44:47 std::make_shared are usually preferable, though he
hub 2017/04/19 14:55:06 let's make it right.
87 jsEngine->SetWebRequest(webRequest);
65 jsEngine->SetFileSystem(AdblockPlus::FileSystemPtr(new LazyFileSystem())); 88 jsEngine->SetFileSystem(AdblockPlus::FileSystemPtr(new LazyFileSystem()));
66 } 89 }
90
91 std::shared_ptr<T> webRequest;
67 }; 92 };
68 93
69 typedef WebRequestTest<MockWebRequest> MockWebRequestTest; 94 typedef WebRequestTest<MockWebRequest> MockWebRequestTest;
70 typedef WebRequestTest<AdblockPlus::DefaultWebRequest> DefaultWebRequestTest; 95 typedef WebRequestTest<AdblockPlus::DefaultWebRequest> DefaultWebRequestTest;
71 typedef WebRequestTest<MockWebRequest> XMLHttpRequestTest; 96 typedef WebRequestTest<MockWebRequest> XMLHttpRequestTest;
72 97
73 void ResetTestXHR(const AdblockPlus::JsEnginePtr& jsEngine) 98 // we return the url of the XHR.
99 std::string ResetTestXHR(const AdblockPlus::JsEnginePtr& jsEngine, const std:: string& defaultUrl = "")
74 { 100 {
75 jsEngine->Evaluate("\ 101 std::string url = defaultUrl;
102 // make up a unique URL if we don't have one.
103 if (url == "")
104 {
105 url = "https://tests.adblockplus.org/easylist.txt-";
106 url += std::to_string(std::chrono::system_clock::to_time_t(std::chrono::sy stem_clock::now()));
107 }
108
109 jsEngine->Evaluate(std::string("\
76 var result;\ 110 var result;\
77 var request = new XMLHttpRequest();\ 111 var request = new XMLHttpRequest();\
78 request.open('GET', 'https://easylist-downloads.adblockplus.org/easylist.t xt');\ 112 request.open('GET', '") + url + "'); \
79 request.overrideMimeType('text/plain');\ 113 request.overrideMimeType('text/plain');\
80 request.addEventListener('load', function() {result = request.responseText ;}, false);\ 114 request.addEventListener('load', function() {result = request.responseText ;}, false);\
81 request.addEventListener('error', function() {result = 'error';}, false);\ 115 request.addEventListener('error', function() {result = 'error';}, false);\
82 "); 116 ");
117 return url;
83 } 118 }
84 119
85 void WaitForVariable(const std::string& variable, const AdblockPlus::JsEngineP tr& jsEngine) 120 void WaitForVariable(const std::string& variable, const AdblockPlus::JsEngineP tr& jsEngine)
86 { 121 {
87 do 122 do
88 { 123 {
89 AdblockPlus::Sleep(60); 124 AdblockPlus::Sleep(60);
90 } while (jsEngine->Evaluate(variable)->IsUndefined()); 125 } while (jsEngine->Evaluate(variable)->IsUndefined());
91 } 126 }
92 127
(...skipping 35 matching lines...) Expand 10 before | Expand all | Expand 10 after
128 #if defined(HAVE_CURL) 163 #if defined(HAVE_CURL)
129 ASSERT_EQ("gzip", jsEngine->Evaluate("foo.responseHeaders['content-encoding']. substr(0, 4)")->AsString()); 164 ASSERT_EQ("gzip", jsEngine->Evaluate("foo.responseHeaders['content-encoding']. substr(0, 4)")->AsString());
130 #endif 165 #endif
131 ASSERT_TRUE(jsEngine->Evaluate("foo.responseHeaders['location']")->IsUndefined ()); 166 ASSERT_TRUE(jsEngine->Evaluate("foo.responseHeaders['location']")->IsUndefined ());
132 } 167 }
133 168
134 TEST_F(DefaultWebRequestTest, XMLHttpRequest) 169 TEST_F(DefaultWebRequestTest, XMLHttpRequest)
135 { 170 {
136 auto filterEngine = AdblockPlus::FilterEngine::Create(jsEngine); 171 auto filterEngine = AdblockPlus::FilterEngine::Create(jsEngine);
137 172
138 ResetTestXHR(jsEngine); 173 ResetTestXHR(jsEngine, "https://easylist-downloads.adblockplus.org/easylist.tx t");
139 jsEngine->Evaluate("\ 174 jsEngine->Evaluate("\
140 request.setRequestHeader('X', 'Y');\ 175 request.setRequestHeader('X', 'Y');\
141 request.setRequestHeader('X2', 'Y2');\ 176 request.setRequestHeader('X2', 'Y2');\
142 request.send(null);"); 177 request.send(null);");
143 WaitForVariable("result", jsEngine); 178 WaitForVariable("result", jsEngine);
144 ASSERT_EQ(AdblockPlus::WebRequest::NS_OK, jsEngine->Evaluate("request.channel. status")->AsInt()); 179 ASSERT_EQ(AdblockPlus::WebRequest::NS_OK, jsEngine->Evaluate("request.channel. status")->AsInt());
145 ASSERT_EQ(200, jsEngine->Evaluate("request.status")->AsInt()); 180 ASSERT_EQ(200, jsEngine->Evaluate("request.status")->AsInt());
146 ASSERT_EQ("[Adblock Plus ", jsEngine->Evaluate("result.substr(0, 14)")->AsStri ng()); 181 ASSERT_EQ("[Adblock Plus ", jsEngine->Evaluate("result.substr(0, 14)")->AsStri ng());
147 ASSERT_EQ("text/plain", jsEngine->Evaluate("request.getResponseHeader('Content -Type').substr(0, 10)")->AsString()); 182 ASSERT_EQ("text/plain", jsEngine->Evaluate("request.getResponseHeader('Content -Type').substr(0, 10)")->AsString());
148 #if defined(HAVE_CURL) 183 #if defined(HAVE_CURL)
(...skipping 59 matching lines...) Expand 10 before | Expand all | Expand 10 after
208 243
209 typedef std::shared_ptr<CatchLogSystem> CatchLogSystemPtr; 244 typedef std::shared_ptr<CatchLogSystem> CatchLogSystemPtr;
210 } 245 }
211 246
212 TEST_F(XMLHttpRequestTest, RequestHeaderValidation) 247 TEST_F(XMLHttpRequestTest, RequestHeaderValidation)
213 { 248 {
214 auto catchLogSystem = CatchLogSystemPtr(new CatchLogSystem()); 249 auto catchLogSystem = CatchLogSystemPtr(new CatchLogSystem());
215 jsEngine->SetLogSystem(catchLogSystem); 250 jsEngine->SetLogSystem(catchLogSystem);
216 251
217 auto filterEngine = AdblockPlus::FilterEngine::Create(jsEngine); 252 auto filterEngine = AdblockPlus::FilterEngine::Create(jsEngine);
218 auto webRequest =
219 std::static_pointer_cast<MockWebRequest>(jsEngine->GetWebRequest());
220 253
221 ASSERT_TRUE(webRequest); 254 ASSERT_TRUE(webRequest);
sergei 2017/04/19 14:44:47 I'm not sure whether we need it.
hub 2017/04/19 14:55:06 I'm probably being paranoid. Even before I'm sure
222 255
223 const std::string msg = "Attempt to set a forbidden header was denied: "; 256 const std::string msg = "Attempt to set a forbidden header was denied: ";
224 257
225 // The test will check that console.warn has been called when the 258 // The test will check that console.warn has been called when the
226 // header is rejected. While this is an implementation detail, we 259 // header is rejected. While this is an implementation detail, we
227 // have no other way to check this 260 // have no other way to check this
228 261
229 // test 'Accept-Encoding' is rejected 262 // test 'Accept-Encoding' is rejected
230 catchLogSystem->clear(); 263 catchLogSystem->clear();
231 ResetTestXHR(jsEngine); 264 std::string url = ResetTestXHR(jsEngine);
232 jsEngine->Evaluate("\ 265 jsEngine->Evaluate("\
233 request.setRequestHeader('Accept-Encoding', 'gzip');\nrequest.send();"); 266 request.setRequestHeader('Accept-Encoding', 'gzip');\nrequest.send();");
234 EXPECT_EQ(AdblockPlus::LogSystem::LOG_LEVEL_WARN, catchLogSystem->lastLogLevel ); 267 EXPECT_EQ(AdblockPlus::LogSystem::LOG_LEVEL_WARN, catchLogSystem->lastLogLevel );
235 EXPECT_EQ(msg + "Accept-Encoding", catchLogSystem->lastMessage); 268 EXPECT_EQ(msg + "Accept-Encoding", catchLogSystem->lastMessage);
236 WaitForVariable("result", jsEngine); 269 WaitForVariable("result", jsEngine);
237 EXPECT_TRUE(webRequest->lastRequestHeaders.cend() == 270 {
238 webRequest->lastRequestHeaders.find("Accept-Encoding")); 271 auto headersRequest = webRequest->headersForRequest(url);
272 EXPECT_TRUE(headersRequest.first);
273 const auto& headers = headersRequest.second;
274 EXPECT_TRUE(headers.cend() == headers.find("Accept-Encoding"));
275 }
239 276
240 // test 'DNT' is rejected 277 // test 'DNT' is rejected
241 catchLogSystem->clear(); 278 catchLogSystem->clear();
242 ResetTestXHR(jsEngine); 279 url = ResetTestXHR(jsEngine);
243 jsEngine->Evaluate("\ 280 jsEngine->Evaluate("\
244 request.setRequestHeader('DNT', '1');\nrequest.send();"); 281 request.setRequestHeader('DNT', '1');\nrequest.send();");
245 EXPECT_EQ(AdblockPlus::LogSystem::LOG_LEVEL_WARN, catchLogSystem->lastLogLevel ); 282 EXPECT_EQ(AdblockPlus::LogSystem::LOG_LEVEL_WARN, catchLogSystem->lastLogLevel );
246 EXPECT_EQ(msg + "DNT", catchLogSystem->lastMessage); 283 EXPECT_EQ(msg + "DNT", catchLogSystem->lastMessage);
247 WaitForVariable("result", jsEngine); 284 WaitForVariable("result", jsEngine);
248 EXPECT_TRUE(webRequest->lastRequestHeaders.cend() == 285 {
249 webRequest->lastRequestHeaders.find("DNT")); 286 auto headersRequest = webRequest->headersForRequest(url);
287 EXPECT_TRUE(headersRequest.first);
288 const auto& headers = headersRequest.second;
289 EXPECT_TRUE(headers.cend() == headers.find("DNT"));
290 }
250 291
251 // test random 'X' header is accepted 292 // test random 'X' header is accepted
252 catchLogSystem->clear(); 293 catchLogSystem->clear();
253 ResetTestXHR(jsEngine); 294 url = ResetTestXHR(jsEngine);
254 jsEngine->Evaluate("\ 295 jsEngine->Evaluate("\
255 request.setRequestHeader('X', 'y');\nrequest.send();"); 296 request.setRequestHeader('X', 'y');\nrequest.send();");
256 EXPECT_EQ(AdblockPlus::LogSystem::LOG_LEVEL_TRACE, catchLogSystem->lastLogLeve l); 297 EXPECT_EQ(AdblockPlus::LogSystem::LOG_LEVEL_TRACE, catchLogSystem->lastLogLeve l);
257 EXPECT_EQ("", catchLogSystem->lastMessage); 298 EXPECT_EQ("", catchLogSystem->lastMessage);
258 WaitForVariable("result", jsEngine); 299 WaitForVariable("result", jsEngine);
259 EXPECT_FALSE(webRequest->lastRequestHeaders.cend() == 300 {
260 webRequest->lastRequestHeaders.find("X")); 301 auto headersRequest = webRequest->headersForRequest(url);
302 EXPECT_TRUE(headersRequest.first);
303 const auto& headers = headersRequest.second;
304 EXPECT_FALSE(headers.cend() == headers.find("X"));
305 }
261 306
262 // test /^Proxy-/ is rejected. 307 // test /^Proxy-/ is rejected.
263 catchLogSystem->clear(); 308 catchLogSystem->clear();
264 ResetTestXHR(jsEngine); 309 url = ResetTestXHR(jsEngine);
265 jsEngine->Evaluate("\ 310 jsEngine->Evaluate("\
266 request.setRequestHeader('Proxy-foo', 'bar');\nrequest.send();"); 311 request.setRequestHeader('Proxy-foo', 'bar');\nrequest.send();");
267 EXPECT_EQ(AdblockPlus::LogSystem::LOG_LEVEL_WARN, catchLogSystem->lastLogLevel ); 312 EXPECT_EQ(AdblockPlus::LogSystem::LOG_LEVEL_WARN, catchLogSystem->lastLogLevel );
268 EXPECT_EQ(msg + "Proxy-foo", catchLogSystem->lastMessage); 313 EXPECT_EQ(msg + "Proxy-foo", catchLogSystem->lastMessage);
269 WaitForVariable("result", jsEngine); 314 WaitForVariable("result", jsEngine);
270 EXPECT_TRUE(webRequest->lastRequestHeaders.cend() == 315 {
271 webRequest->lastRequestHeaders.find("Proxy-foo")); 316 auto headersRequest = webRequest->headersForRequest(url);
317 EXPECT_TRUE(headersRequest.first);
318 const auto& headers = headersRequest.second;
319 EXPECT_TRUE(headers.cend() == headers.find("Proxy-foo"));
320 }
272 321
273 // test /^Sec-/ is rejected. 322 // test /^Sec-/ is rejected.
274 catchLogSystem->clear(); 323 catchLogSystem->clear();
275 ResetTestXHR(jsEngine); 324 url = ResetTestXHR(jsEngine);
276 jsEngine->Evaluate("\ 325 jsEngine->Evaluate("\
277 request.setRequestHeader('Sec-foo', 'bar');\nrequest.send();"); 326 request.setRequestHeader('Sec-foo', 'bar');\nrequest.send();");
278 EXPECT_EQ(AdblockPlus::LogSystem::LOG_LEVEL_WARN, catchLogSystem->lastLogLevel ); 327 EXPECT_EQ(AdblockPlus::LogSystem::LOG_LEVEL_WARN, catchLogSystem->lastLogLevel );
279 EXPECT_EQ(msg + "Sec-foo", catchLogSystem->lastMessage); 328 EXPECT_EQ(msg + "Sec-foo", catchLogSystem->lastMessage);
280 WaitForVariable("result", jsEngine); 329 WaitForVariable("result", jsEngine);
281 EXPECT_TRUE(webRequest->lastRequestHeaders.cend() == 330 {
282 webRequest->lastRequestHeaders.find("Sec-foo")); 331 auto headersRequest = webRequest->headersForRequest(url);
332 EXPECT_TRUE(headersRequest.first);
333 const auto& headers = headersRequest.second;
334 EXPECT_TRUE(headers.cend() == headers.find("Sec-foo"));
335 }
283 336
284 // test 'Security' is accepted. 337 // test 'Security' is accepted.
285 catchLogSystem->clear(); 338 catchLogSystem->clear();
286 ResetTestXHR(jsEngine); 339 url = ResetTestXHR(jsEngine);
287 jsEngine->Evaluate("\ 340 jsEngine->Evaluate("\
288 request.setRequestHeader('Security', 'theater');\nrequest.send();"); 341 request.setRequestHeader('Security', 'theater');\nrequest.send();");
289 EXPECT_EQ(AdblockPlus::LogSystem::LOG_LEVEL_TRACE, catchLogSystem->lastLogLeve l); 342 EXPECT_EQ(AdblockPlus::LogSystem::LOG_LEVEL_TRACE, catchLogSystem->lastLogLeve l);
290 EXPECT_EQ("", catchLogSystem->lastMessage); 343 EXPECT_EQ("", catchLogSystem->lastMessage);
291 WaitForVariable("result", jsEngine); 344 WaitForVariable("result", jsEngine);
292 EXPECT_FALSE(webRequest->lastRequestHeaders.cend() == 345 {
293 webRequest->lastRequestHeaders.find("Security")); 346 auto headersRequest = webRequest->headersForRequest(url);
347 EXPECT_TRUE(headersRequest.first);
348 const auto& headers = headersRequest.second;
349 EXPECT_FALSE(headers.cend() == headers.find("Security"));
350 }
294 } 351 }
OLDNEW
« no previous file with comments | « no previous file | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld