Re: [webkit-dev] Eliminate potential null pointer dereference?
On Apr 19, 2012, at 10:35 PM, David Barr wrote: On Fri, Apr 20, 2012 at 3:18 PM, Alexey Proskuryakov a...@webkit.org wrote: I noticed a number of patches going in recently that add null checks, or refactor the code under the premise of eliminating potential null dereference. What does that mean, and why are we allowing such patches to be landed? When there are known conditions for which a value is null that we would otherwise dereference, check first and take an alternate path in the null case. For example, this change doesn't look nice: http://trac.webkit.org/changeset/114678/trunk/Source/WebCore/css/CSSStyleSelector.cpp. We should not try to confuse ourselves with unusual for loop forms, and there is no explanation at all of why these changes are needed. No regression tests either. Style aside, it is quite clear that the one of the termination conditions for the first loop is selector == null. So the second loop ought to check selector != null before any dereference of selector. If that code path is reachable, then it should be possible to construct a test, and the claim that there's no new tests because it's code cleanup only is wrong. If it is not reachable, then your argument does not apply Regarding style, the change homogenizes the loop constructs within that method. That seems subjective. Making a bunch of these tiny changes that are not tied to an actual change in behavior or a clear-cut broader goal has some downsides: - Makes it harder to study history - Causes needless extra build thrash for people and buildbots - Creates risk of accidentally introducing a bug I don't have a strong opinion on this one, but if a bunch of these changes are landing, then either they need tests if they fix real bugs, or they should be related to some more concrete goal than just code cleanup. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] How can I get some debug infomation in MediaPlayerPrivateGStreamer.cpp
hi Terry,
In fact, I do something like this below:
MediaPlayerPrivateInterface*
MediaPlayerPrivateGStreamer::create(MediaPlayer* player)
{
printf(MediaPlayerPrivateGStreamer::create(MediaPlayer* player) is
called\n);// added by zqhuan
return new MediaPlayerPrivateGStreamer(player);
}
void MediaPlayerPrivateGStreamer::registerMediaEngine(MediaEngineRegistrar
registrar)
{
printf(MediaPlayerPrivateGStreamer::registerMediaEngine(MediaEngineRegistrar
registrar)\n);//added by zqhuan
if (isAvailable())
registrar(create, getSupportedTypes, supportsType);
}
bool MediaPlayerPrivateGStreamer::isAvailable()
{
printf(isAvailable() is called\n); //added by zqhuan
if (!doGstInit())
return false;
GstElementFactory* factory = gst_element_factory_find(playbin2);
if (factory) {
gst_object_unref(GST_OBJECT(factory));
return true;
}
return false;
}
but when I test with this, got nothing output.
And then, I add ASSERT(false) after the printf statement in
registerMediaEngine function, I can see the output:
MediaPlayerPrivateGStreamer::registerMediaEngine(MediaEngineRegistrar
registrar)
isAvailable() is called
I don't know why this happen. Any ideas?
Best regards,
zqhuan
在 2012年4月20日 上午11:35，Terry Anderson tdander...@google.com写道：
Are you sure that the code is even being reached? Try an ASSERT(false); to
make sure.
Terry
On Apr 19, 2012 7:42 PM, ZiQiangHuan hzqh...@gmail.com wrote:
hi，
I build webkit with gstreamer support, but I encountered a problem when
I test with it. When I write a simple statement printf(*\n) in the
function isAvailable() of MediaPlayerPrivateGStreamer.cpp, I got nothing
output. When I do the same thing with the function create of
MediaPlayerPrivateGStreamer.cpp, I got nothing output too. So I want to
know if I want to get some debug info in these functions, what should I do?
anyone has some experience with this? And I don't know why this happen.
Any help would be appreciated.
Best regards,
zqhuan
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Eliminate potential null pointer dereference?
I think this all started with a lot of effort put into fixing an issue reported by a user where they said the most popular online forum in Malaysia is broken. Then folks had to do a lot of builds (bisecting) to track down where the problem was introduced. Then they had to figure out what had broken, etc. It was mentioned (by gr...@chromium.org) that this very issue had already been flagged by own internal runs of coverity on chromium (including webkit). Now, it seemed a shame that we knew about issues in WebKit and were just ignoring them. It would be nice to be able to catch these issues faster rather than wait for a user to report it, etc. which makes the expense overall go up. So I believe there has been some effort invested in fixing some issues pointed out by coverity which is what these changes are and I believe coverity is mentioned in other changes of this sort. I understand the other side as well that it would be good to figure out if it is really an issue and find a test to prove it. I guess this is more of what I think of as a BSD type of approach. It seems to be an area where reasonable people can disagree. oth, regarding the style of this particular change, I find it unusual as well. dave ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Eliminate potential null pointer dereference?
On Thu, Apr 19, 2012 at 10:35 PM, David Barr davidb...@google.com wrote: Regarding style, the change homogenizes the loop constructs within that method. I don't think that's necessary an improvement. e.g. we don't have a style guide that mandates it. I completely agree with Maciej here that if this is a reachable code, then the patch author should put a reasonable effort into creating a test case. And most importantly, these changes are clearly not code cleanup. On Thu, Apr 19, 2012 at 11:11 PM, David Levin le...@google.com wrote: I understand the other side as well that it would be good to figure out if it is really an issue and find a test to prove it. I guess this is more of what I think of as a BSD type of approach. It seems to be an area where reasonable people can disagree. The WebKit contribution guide lists this as a requirement ( http://www.webkit.org/coding/contributing.html): For any feature that affects the layout engine, a new regression test must be constructed. If you provide a patch that fixes a bug, that patch should also include the addition of a regression test that would fail without the patch and succeed with the patch. If no regression test is provided, the reviewer will ask you to revise the patch, so you can save time by constructing the test up front and making sure it's attached to the bug. So I don't think we can disagree on this topic. I'm sympathetic to the argument that it's hard to come up with a test case for things like this, but then the patch author should clearly state that in the change log, and most importantly the reviewer should be asking that during the review. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Eliminate potential null pointer dereference?
On Thu, Apr 19, 2012 at 11:36 PM, Ryosuke Niwa rn...@webkit.org wrote: On Thu, Apr 19, 2012 at 10:35 PM, David Barr davidb...@google.com wrote: Regarding style, the change homogenizes the loop constructs within that method. I don't think that's necessary an improvement. e.g. we don't have a style guide that mandates it. I completely agree with Maciej here that if this is a reachable code, then the patch author should put a reasonable effort into creating a test case. And most importantly, these changes are clearly not code cleanup. On Thu, Apr 19, 2012 at 11:11 PM, David Levin le...@google.com wrote: I understand the other side as well that it would be good to figure out if it is really an issue and find a test to prove it. I guess this is more of what I think of as a BSD type of approach. It seems to be an area where reasonable people can disagree. The WebKit contribution guide lists this as a requirement ( http://www.webkit.org/coding/contributing.html): For any feature that affects the layout engine, a new regression test must be constructed. If you provide a patch that fixes a bug, that patch should also include the addition of a regression test that would fail without the patch and succeed with the patch. If no regression test is provided, the reviewer will ask you to revise the patch, so you can save time by constructing the test up front and making sure it's attached to the bug. So I don't think we can disagree on this topic. I'm sympathetic to the argument that it's hard to come up with a test case for things like this, but then the patch author should clearly state that in the change log, and most importantly the reviewer should be asking that during the review. You seem to be a bit confrontational here. I'm not sure why. I was talking about about doing a patch for something where one wasn't able to find a repro but it appeared like an issue might be there. Not whether the changelog should say that or not. It may be good to ask a clarifying question if something is unclear as opposed to responding like this. dave ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Where to call Codeblock::dump from within webcore
On Wed 18 Apr 2012 21:28, pgsery pgs...@swcp.com writes: I want to call CodeBlock::dump from webcore in addition to the jsc shell. I've compiled webkit-1.6.1 with the --enable-debug option, modified dump to print to a file, instead of stdout, and set options.dump=true. This works from the shell, but now I need to find where to call dump from within webkit. If I understand you correctly, call ByteCompiler::setDumpsGeneratedCode(true). Note that in the latest nightlies, this is available in regular builds. Andy -- http://wingolog.org/ ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Eliminate potential null pointer dereference?
Changing the two loops to use the same form improves readability because it makes it clear that the form of iteration is the same between the two loops. This is a very common C pattern when dealing with lists where the behavior changes after an element is encountered. The pattern is used instead of a flag because it eliminates branches in the code. In this case, not using the canonical two-for-loop form has lead the original writer to create code that is not obviously correct - you can’t look at this function and see that it does what it claims - in fact, the most obvious code path of exiting the first loop through the while condition directly results in a null pointer dereference. Code should communicate meaning to other software engineers. It should allow us to reason together about the code, and to have certainty about what the code will do. I don’t think anyone could look at the existing code and be assured that it was correct. Fixing that problem and clearly communicating the correctness of the code is not “code churn” - on the contrary it is the kind of change that is vital to the long-term health of the codebase. Tests are a good thing, but they are not the only thing. Consider the state-space of a large piece of software like webkit - it is essentially infinite. You can’t test every case and code path to ensure correctness. On the other hand, we don’t yet have the tools to produce a proof of correctness for webkit, and so we live somewhere between the two - some of our confidence comes from being able to reason about the correctness of the code in general, and some of our confidence comes from being able to test a small fraction of the state space. What do we hope to achieve by adding a test when fixing a bug? To prevent a bug from being reintroduced into the codebase at a later date. This is a reasonable goal, so let’s remember that the goal is to prevent the bug from recurring, not to add a test for its own sake. In this case, the potential null pointer dereference was found using coverity, a static analysis tool that we run nightly. If the bug were to be reintroduced it is reasonable to expect that static analysis would be able to find it again. Hope this helps you see where I'm coming from, Luke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Eliminate potential null pointer dereference?
On Fri, Apr 20, 2012 at 10:53 AM, Luke Macpherson macpher...@chromium.orgwrote: Tests are a good thing, but they are not the only thing. Consider the state-space of a large piece of software like webkit - it is essentially infinite. You can’t test every case and code path to ensure correctness. While I do understand where you're coming from, this is an agreed policy. We should state why tests are absent in change logs or in bugs when it's hard to create one putting reasonable efforts into creating one. This is a reasonable goal, so let’s remember that the goal is to prevent the bug from recurring, not to add a test for its own sake. In this case, the potential null pointer dereference was found using coverity, a static analysis tool that we run nightly. Is the code reachable? It's quite possible that the code is unreachable and therefore there is no way to hit that crash. Without a test, we can't answer that question. If the bug were to be reintroduced it is reasonable to expect that static analysis would be able to find it again. WebKit contributors are not required to use such a tool prior to committing their changes at least for now, but we DO require contributors to run our layout tests. And I don't think we want to require all contributors to run coverity on webkit before committing their patches. Given that, there is definitely a benefit in adding a test case for simple fixes like this. On the other hand, I don't think it's realistic to force contributors to come up with a test case if it's really hard since that could become a significant development overhead as well. And in those cases where we decide that it's too hard to create a test, we explicitly mention it in the change log. It is this piece the particular patch ap brought up is missing. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Eliminate potential null pointer dereference?
On Fri, Apr 20, 2012 at 11:07 AM, Ryosuke Niwa rn...@webkit.org wrote: Is the code reachable? It's quite possible that the code is unreachable and therefore there is no way to hit that crash. Without a test, we can't answer that question. That is not rationally true. A test case can show that there is a code path leading to a null pointer dereference. A test cannot show that there are no possible code paths that lead to that state. This is exactly what I was getting at when explaining that the state space of webkit is too large to test. In this case we don't have a repro case that leads to that state, but that does not mean that it is not possible, or that the potential to crash should not be fixed. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Eliminate potential null pointer dereference?
On Fri, Apr 20, 2012 at 10:53 AM, Luke Macpherson macpher...@chromium.org wrote: What do we hope to achieve by adding a test when fixing a bug? To prevent a bug from being reintroduced into the codebase at a later date. This is a reasonable goal, so let’s remember that the goal is to prevent the bug from recurring, not to add a test for its own sake. In this case, the potential null pointer dereference was found using coverity, a static analysis tool that we run nightly. If the bug were to be reintroduced it is reasonable to expect that static analysis would be able to find it again. I remember seeing similar patch based on Coverty, and when asked for a test, the author discovered the null check was useless in that place and the code was changed differently. I wholeheartedly agree we should try to make tests for code changed based on static code analyzer. This will help making sure the change is meaningful, and the test would prevent regression. Benjamin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Eliminate potential null pointer dereference?
+1 to the idea that we should try to add a test for each change. That being said, as rniwa mentioned, it is sometimes difficult to make a test because (A) we do not come up with a test case (B) we know that the code is unreachable (e.g. https://bugs.webkit.org/show_bug.cgi?id=84377) What is the consensus for such cases? As long as the change improves codebase and the benefit is explicitly described in ChangeLog, is it OK to commit the patch? Or shouldn't we try to fix a potential bug observed in static analysis? On Fri, Apr 20, 2012 at 11:40 AM, Benjamin Poulain benja...@webkit.org wrote: On Fri, Apr 20, 2012 at 10:53 AM, Luke Macpherson macpher...@chromium.org wrote: What do we hope to achieve by adding a test when fixing a bug? To prevent a bug from being reintroduced into the codebase at a later date. This is a reasonable goal, so let’s remember that the goal is to prevent the bug from recurring, not to add a test for its own sake. In this case, the potential null pointer dereference was found using coverity, a static analysis tool that we run nightly. If the bug were to be reintroduced it is reasonable to expect that static analysis would be able to find it again. I remember seeing similar patch based on Coverty, and when asked for a test, the author discovered the null check was useless in that place and the code was changed differently. I wholeheartedly agree we should try to make tests for code changed based on static code analyzer. This will help making sure the change is meaningful, and the test would prevent regression. Benjamin -- Kentaro Hara, Tokyo, Japan (http://haraken.info) ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Eliminate potential null pointer dereference?
On Apr 20, 2012, at 11:40 AM, Benjamin Poulain benja...@webkit.org wrote: On Fri, Apr 20, 2012 at 10:53 AM, Luke Macpherson macpher...@chromium.org wrote: What do we hope to achieve by adding a test when fixing a bug? To prevent a bug from being reintroduced into the codebase at a later date. This is a reasonable goal, so let’s remember that the goal is to prevent the bug from recurring, not to add a test for its own sake. In this case, the potential null pointer dereference was found using coverity, a static analysis tool that we run nightly. If the bug were to be reintroduced it is reasonable to expect that static analysis would be able to find it again. I remember seeing similar patch based on Coverty, and when asked for a test, the author discovered the null check was useless in that place and the code was changed differently. I wholeheartedly agree we should try to make tests for code changed based on static code analyzer. This will help making sure the change is meaningful, and the test would prevent regression. +1 Someone in this thread said something about code readability. So let's consider that. If I see code like: if (!var) thingy(); Then I will be under the impression that var might sometimes be zero and that thingy() might sometimes happen, and that the previous webkittens to touch this code had a good reason for this check. Coverity is no more accurate than testing; if it tells you that var might be zero than it cannot, will not, and does not give you 100% confidence that this is reachable. Hence if you add special cases for things that coverity warned you about, you are potentially doing a disservice to anyone looking at the code in the future. You are telling them that var might be zero when really you meant to say I put in this check to get my tool to stop complaining. On the other hand, if you are able to construct a test that demonstrates reachability then you win. But if there is no test then see my previous paragraph. -F ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Eliminate potential null pointer dereference?
On Fri, Apr 20, 2012 at 3:06 PM, Filip Pizlo fpi...@apple.com wrote: Someone in this thread said something about code readability. So let's consider that. If I see code like: if (!var) thingy(); Then I will be under the impression that var might sometimes be zero and that thingy() might sometimes happen, and that the previous webkittens to touch this code had a good reason for this check. Coverity is no more accurate than testing; if it tells you that var might be zero than it cannot, will not, and does not give you 100% confidence that this is reachable. Hence if you add special cases for things that coverity warned you about, you are potentially doing a disservice to anyone looking at the code in the future. You are telling them that var might be zero when really you meant to say I put in this check to get my tool to stop complaining. On the other hand, if you are able to construct a test that demonstrates reachability then you win. But if there is no test then see my previous paragraph. Wouldn't an ASSERT be more helpful in this case, i.e. something like: ASSERT(var); thingy(); Does ASSERT make coverity stop giving out warnings? ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] How can I get some debug infomation in MediaPlayerPrivateGStreamer.cpp
On Fri, 2012-04-20 at 10:42 +0800, ZiQiangHuan wrote: hi， I build webkit with gstreamer support, but I encountered a problem when I test with it. When I write a simple statement printf(* \n) in the function isAvailable() of MediaPlayerPrivateGStreamer.cpp, I got nothing output. When I do the same thing with the function create of MediaPlayerPrivateGStreamer.cpp, I got nothing output too. So I want to know if I want to get some debug info in these functions, what should I do? anyone has some experience with this? And I don't know why this happen. That's odd, the stdout buffer should be flushed since you insert a new line. Try to force a fflush() call maybe? Philippe signature.asc Description: This is a digitally signed message part ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Eliminate potential null pointer dereference?
On Apr 19, 2012, at 11:11 PM, David Levin wrote: I think this all started with a lot of effort put into fixing an issue reported by a user where they said the most popular online forum in Malaysia is broken. Then folks had to do a lot of builds (bisecting) to track down where the problem was introduced. Then they had to figure out what had broken, etc. It was mentioned (by gr...@chromium.org) that this very issue had already been flagged by own internal runs of coverity on chromium (including webkit). Now, it seemed a shame that we knew about issues in WebKit and were just ignoring them. It would be nice to be able to catch these issues faster rather than wait for a user to report it, etc. which makes the expense overall go up. So I believe there has been some effort invested in fixing some issues pointed out by coverity which is what these changes are and I believe coverity is mentioned in other changes of this sort. I understand the other side as well that it would be good to figure out if it is really an issue and find a test to prove it. I guess this is more of what I think of as a BSD type of approach. It seems to be an area where reasonable people can disagree. oth, regarding the style of this particular change, I find it unusual as well. If the change was attempting to fix an issue found by a specific static analyzer, then it should say so in the ChangeLog instead of No new tests / code cleanup only. That goes double if a lot of such changes are being made, or people not in the know will be really confused (I don't think Alexey was the only one). Also, the fact that it was found by a static analyzer does not exempt the contributor from making a reasonable effort to create a test case. We do accept that sometimes it's not practical and in that case it's ok to mention in the ChangeLog why it was not possible to make a test case. However, code cleanup only is not a reason that applies to changes made with the intent of producing a behavior change. If we had a static analyzer that ran automatically as part of the WebKit development process, and a shared goal to get its complaints down to 0, then it might be reasonable to skip creating tests for issues that it diagnoses. But that doesn't seem to be the situation here. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Eliminate potential null pointer dereference?
On Fri, Apr 20, 2012 at 1:39 PM, Maciej Stachowiak m...@apple.com wrote:
On Apr 19, 2012, at 11:11 PM, David Levin wrote:
I think this all started with a lot of effort put into fixing an issue
reported by a user where they said the most popular online forum in
Malaysia is broken. Then folks had to do a lot of builds (bisecting) to
track down where the problem was introduced. Then they had to figure out
what had broken, etc.
It was mentioned (by gr...@chromium.org) that this very issue had already
been flagged by own internal runs of coverity on chromium (including
webkit). Now, it seemed a shame that we knew about issues in WebKit and
were just ignoring them. It would be nice to be able to catch these issues
faster rather than wait for a user to report it, etc. which makes the
expense overall go up.
So I believe there has been some effort invested in fixing some issues
pointed out by coverity which is what these changes are and I believe
coverity is mentioned in other changes of this sort.
I understand the other side as well that it would be good to figure out if
it is really an issue and find a test to prove it. I guess this is more of
what I think of as a BSD type of approach. It seems to be an area where
reasonable people can disagree.
oth, regarding the style of this particular change, I find it unusual as
well.
If the change was attempting to fix an issue found by a specific static
analyzer, then it should say so in the ChangeLog instead of No new tests /
code cleanup only. That goes double if a lot of such changes are being
made, or people not in the know will be really confused (I don't think
Alexey was the only one).
Totally agreed. I think that was a mistake here. (I haven't been involved
in this error but I did happen to notice other patches that mentioned
coverity. This would should have as well.)
Also, the fact that it was found by a static analyzer does not exempt the
contributor from making a reasonable effort to create a test case. We do
accept that sometimes it's not practical and in that case it's ok to
mention in the ChangeLog why it was not possible to make a test case.
However, code cleanup only is not a reason that applies to changes made
with the intent of producing a behavior change.
Yes there should have been a reasonable effort.
If we had a static analyzer that ran automatically as part of the WebKit
development process, and a shared goal to get its complaints down to 0,
then it might be reasonable to skip creating tests for issues that it
diagnoses. But that doesn't seem to be the situation here.
I wonder why there would need this criteria. Is it better to leave in
obvious wrong code?
In this case the code looked like approximately like this before the change:
while (foo) {
}
for (foo = foo-next();...)
Now either that while loop should be while (true) or else foo should be
null checked. Ideally, this code wouldn't have passed the 1st code review
(and it is unlikely that a test would have been required to do the fix at
that stage).
dave
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Eliminate potential null pointer dereference?
Hi Luke, I feel like you've been put on the defensive a bit here, which is unfortunate. I do agree with the value of readable code and hackability is one of the core values of the WebKit project. Code should indeed be meaningful to programmers. One thing to keep in mind though is that, by this definition, the standard of clean code is partly subjective. If other developers on the project do not in fact find changes to be more readable, then it's not actually a readability improvement. As far as regression tests vs. static analyzers: the goal of regression tests is not only to prevent bugs from recurring but to have a shared way for the community as a whole to do so. A static analyzer might be an adequate substitute in some cases if others (a) knew about it and (b) agreed that it should become one of the project's testing tools. In other words, it's not the form of the test that's key, it's the knowledge and general availability of the relevant testing tool. Finally, one additional comment on another email of yours: On Apr 20, 2012, at 11:25 AM, Luke Macpherson wrote: On Fri, Apr 20, 2012 at 11:07 AM, Ryosuke Niwa rn...@webkit.org wrote: Is the code reachable? It's quite possible that the code is unreachable and therefore there is no way to hit that crash. Without a test, we can't answer that question. That is not rationally true. A test case can show that there is a code path leading to a null pointer dereference. A test cannot show that there are no possible code paths that lead to that state. This is exactly what I was getting at when explaining that the state space of webkit is too large to test. In this case we don't have a repro case that leads to that state, but that does not mean that it is not possible, or that the potential to crash should not be fixed. The WebKit project's regression test policy does not require you to prove a universal negative, just to make a test that provides reasonable coverage, would have failed without the change, and passes with it. Historically as a project, we have not chosen to give up on testing even though perfect testing is infeasible. Regards, Maciej On Apr 20, 2012, at 10:53 AM, Luke Macpherson wrote: Changing the two loops to use the same form improves readability because it makes it clear that the form of iteration is the same between the two loops. This is a very common C pattern when dealing with lists where the behavior changes after an element is encountered. The pattern is used instead of a flag because it eliminates branches in the code. In this case, not using the canonical two-for-loop form has lead the original writer to create code that is not obviously correct - you can’t look at this function and see that it does what it claims - in fact, the most obvious code path of exiting the first loop through the while condition directly results in a null pointer dereference. Code should communicate meaning to other software engineers. It should allow us to reason together about the code, and to have certainty about what the code will do. I don’t think anyone could look at the existing code and be assured that it was correct. Fixing that problem and clearly communicating the correctness of the code is not “code churn” - on the contrary it is the kind of change that is vital to the long-term health of the codebase. Tests are a good thing, but they are not the only thing. Consider the state-space of a large piece of software like webkit - it is essentially infinite. You can’t test every case and code path to ensure correctness. On the other hand, we don’t yet have the tools to produce a proof of correctness for webkit, and so we live somewhere between the two - some of our confidence comes from being able to reason about the correctness of the code in general, and some of our confidence comes from being able to test a small fraction of the state space. What do we hope to achieve by adding a test when fixing a bug? To prevent a bug from being reintroduced into the codebase at a later date. This is a reasonable goal, so let’s remember that the goal is to prevent the bug from recurring, not to add a test for its own sake. In this case, the potential null pointer dereference was found using coverity, a static analysis tool that we run nightly. If the bug were to be reintroduced it is reasonable to expect that static analysis would be able to find it again. Hope this helps you see where I'm coming from, Luke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Eliminate potential null pointer dereference?
On Apr 20, 2012, at 12:05 PM, Kentaro Hara wrote: +1 to the idea that we should try to add a test for each change. That being said, as rniwa mentioned, it is sometimes difficult to make a test because (A) we do not come up with a test case (B) we know that the code is unreachable (e.g. https://bugs.webkit.org/show_bug.cgi?id=84377) What is the consensus for such cases? As long as the change improves codebase and the benefit is explicitly described in ChangeLog, is it OK to commit the patch? Or shouldn't we try to fix a potential bug observed in static analysis? FWIW the change cited above and its current ChangeLog seem ok to me. In some cases, it's useful and important for a constructor to leave a data member uninitialized (e.g. for performance reasons), but those cases are exceptions and would need to be documented. The ChangeLog also explains how the issue fixed here is unreachable in practice and also untestable. If this issue was spotted by a static analyzer, it would be good to mention that in the ChangeLog too. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Eliminate potential null pointer dereference?
If we had a static analyzer that ran automatically as part of the WebKit development process, and a shared goal to get its complaints down to 0, then it might be reasonable to skip creating tests for issues that it diagnoses. But that doesn't seem to be the situation here. If we ran a static analyzer as part of the process with the goal of having cleaner code, we could have demonstrably avoided at least one bug with a big enough impact to avoid a hot patch. Mind, I'm not advocating doing that. I'm aware that false positives and a lot of noise bugs make this a very difficult goal to achieve. What we currently do strikes me as the better approach - we run the analyzer, and people who actually care about that kind of stuff triage and fix the remaining actual issues. (And believe me, we triage out quite a bit :) Rachel ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Eliminate potential null pointer dereference?
I completely agree with Maciej here that if this is a reachable code, then the patch author should put a reasonable effort into creating a test case. And most importantly, these changes are clearly not code cleanup. I'm disagreeing here. (as far as NULL checks go). Unless there's a demonstrable reason that you _need_ a value uninitialized, why is the burden of proof on the person doing cleanup? Yes, at the point the code was written, it's well possible that the author was aware that the value would always be initialized for use. However, if code is added to a class, that invariant is not always checked again. I think the confusion is over the intent of the person making the cleanup change. We (I speak as one of the people pushing static analysis) are not interested in *changing* WebKit behavior. We're not fixing existing bugs. We're interested in making sure behavior is deterministic. Requiring the construction of what amounts to an exploit for each fix for uninitialized variables seems a bit overkill :) I agree that the CHANGELOG entry should state that we deliberately didn't add tests. My personal policy is to propose those patches, complete with No new tests/ cleanup only. If I get a lot of pushback on the review, I'm happy to abandon it. Rachel ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Eliminate potential null pointer dereference?
I completely agree with Maciej here that if this is a reachable code, then the patch author should put a reasonable effort into creating a test case. And most importantly, these changes are clearly not code cleanup. I'm disagreeing here. (as far as NULL checks go). Unless there's a demonstrable reason that you _need_ a value uninitialized, why is the burden of proof on the person doing cleanup? Yes, at the point the code was written, it's well possible that the author was aware that the value would always be initialized for use. However, if code is added to a class, that invariant is not always checked again. I think the confusion is over the intent of the person making the cleanup change. We (I speak as one of the people pushing static analysis) are not interested in *changing* WebKit behavior. We're interested in making sure behavior is deterministic. Requiring the construction of what amounts to an exploit for each fix for uninitialized variables seems a bit overkill :) I agree that the CHANGELOG entry should state that we deliberately didn't add tests. My personal policy is to propose those patches, complete with No new tests/ cleanup only. If I get pushback on the review, I'm happy to abandon it. Rachel On Thu, Apr 19, 2012 at 11:11 PM, David Levin le...@google.com wrote: I understand the other side as well that it would be good to figure out if it is really an issue and find a test to prove it. I guess this is more of what I think of as a BSD type of approach. It seems to be an area where reasonable people can disagree. The WebKit contribution guide lists this as a requirement ( http://www.webkit.org/coding/contributing.html): For any feature that affects the layout engine, a new regression test must be constructed. If you provide a patch that fixes a bug, that patch should also include the addition of a regression test that would fail without the patch and succeed with the patch. If no regression test is provided, the reviewer will ask you to revise the patch, so you can save time by constructing the test up front and making sure it's attached to the bug. So I don't think we can disagree on this topic. I'm sympathetic to the argument that it's hard to come up with a test case for things like this, but then the patch author should clearly state that in the change log, and most importantly the reviewer should be asking that during the review. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Eliminate potential null pointer dereference?
If this issue was spotted by a static analyzer, it would be good to mention that in the ChangeLog too. We (the static analysis loons ;) usually try to do that. (The log will contain CID= to identify the Coverity ID, and most of the time a [Coverity] tag). The bug that David mentioned above got Coverity a lot more attention on the Chromium team, and quite a few people started working with it, most likely without knowing we do that. I apologize I didn't clearly communicate that's a best practice when I pointed people there. Updating our documentation as we speak. Rachel Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Eliminate potential null pointer dereference?
On Apr 20, 2012, at 1:48 PM, Rachel Blum wrote: I completely agree with Maciej here that if this is a reachable code, then the patch author should put a reasonable effort into creating a test case. And most importantly, these changes are clearly not code cleanup. I'm disagreeing here. (as far as NULL checks go). Unless there's a demonstrable reason that you _need_ a value uninitialized, why is the burden of proof on the person doing cleanup? Yes, at the point the code was written, it's well possible that the author was aware that the value would always be initialized for use. However, if code is added to a class, that invariant is not always checked again. I think there's a difference between null checks and initializing variables. I think it would be a reasonable style guideline to say that a constructor must initialize every data member, except possibly ones that have their own constructors or cases where the reason not to initialize is documented. I don't know if I'd agree with a style guideline that says add a null check everywhere that it seems like a value might be null. I think the confusion is over the intent of the person making the cleanup change. We (I speak as one of the people pushing static analysis) are not interested in *changing* WebKit behavior. We're interested in making sure behavior is deterministic. Requiring the construction of what amounts to an exploit for each fix for uninitialized variables seems a bit overkill :) I agree that the CHANGELOG entry should state that we deliberately didn't add tests. My personal policy is to propose those patches, complete with No new tests/ cleanup only. If I get pushback on the review, I'm happy to abandon it. I think given our current project policies, the best practice would be: - Identify that the change was made in response to a static analyzer (and identify the tool) - If the patch would cause a behavior change, make a reasonable attempt to make a test - If the code path is in practice unreachable, then document that, and also think about whether the way it's addressed makes that clear. - If you can't determine either way, then document that. I don't know of a way to reach this code path but I can't prove it's impossible is more accurate than No new tests/cleanup only. You mentioned in another email that you're not in favor of blindly doing everything the tool says, so I think that the above is a reasonable thing to expect as part of thinking about the tool's output. To take the example of the last patch here, if the variable in question can't actually be null on exit from the first loop, the right fix would have been to change the loop condition rather than to add a null check. That would have made the code more readable for future programmers. If it is reachable, then the test would have helped people who do not have access to the relevant analysis tool, or the same level of understanding about which of its messages to obey and which to ignore. So I think the expectations here are reasonable. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Eliminate potential null pointer dereference?
On Fri, Apr 20, 2012 at 1:48 PM, Rachel Blum gr...@chromium.org wrote: Unless there's a demonstrable reason that you _need_ a value uninitialized, why is the burden of proof on the person doing cleanup? Yes, at the point the code was written, it's well possible that the author was aware that the value would always be initialized for use. However, if code is added to a class, that invariant is not always checked again. Unless totally trivial, a patch doing a cleanup is no different than a patch adding a feature or fixing a bug. You should demonstrate your change is correct. The patch might be great, or it might be bad and blindly following a tool. You show it is correct by adding tests or having a good Changelog. Benjamin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] Hackathon: Convert pixel tests to text-only tests
Hi all, I've put together a list of DRT tests that may be convertible to either text or ref tests: https://docs.google.com/spreadsheet/ccc?key=0Aju75XfChxj-dGJKd2tZcm84anliQ3NXNS1wTWtQR0E. Feel free to either choose a test yourself or pick one of the tests from the list and convert it. Dan ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Contributor meeting hackathon: Move methods from layoutTestController
I've long felt guilty about setPOSIXLocale, which was used to regression test an issue that only comes up on non-Qt Linux ports (related to how sprintf handled floating-point numbers dependent on locale). But from a glance at the code it may be used in other places now. On Thu, Apr 19, 2012 at 4:54 PM, Alexey Proskuryakov a...@webkit.org wrote: We're capturing ideas for which LayoutTestController methods could be moved to Internals here: https://trac.webkit.org/wiki/Internals_Hackathon. Even found some that are completely unused in tests! - WBR, Alexey Proskuryakov ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Where to call Codeblock::dump from within webcore
On 04/20/2012 08:25 AM, Andy Wingo wrote: On Wed 18 Apr 2012 21:28, pgserypgs...@swcp.com writes: I want to call CodeBlock::dump from webcore in addition to the jsc shell. I've compiled webkit-1.6.1 with the --enable-debug option, modified dump to print to a file, instead of stdout, and set options.dump=true. This works from the shell, but now I need to find where to call dump from within webkit. If I understand you correctly, call ByteCompiler::setDumpsGeneratedCode(true). Note that in the latest nightlies, this is available in regular builds. Andy Yes, that's what I want to do. I'm calling setDumpsGeneratedCode(true) from main in jsc.cpp to force the dump from the shell without using the -d option. However, I also want to force webkit to use this option without having to set it in the shell. For instance, I'm using the python bindings in pywebkit to use webkit and would like (I think) to embed setDumpsGeneratedCode(true) in the appropriate fn in libjavascriptcoregtk or libwebkitgtk. Or perhaps I've missed a way of using an API call or some other way. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] How can I get some debug infomation in MediaPlayerPrivateGStreamer.cpp
hi Philippe, Thank you for your reply. In some functions of MediaPlayerPrivateGStreamer.cpp, I can get output of printf. But when I add printf in some functions just like isAvailable, I cannot get anything output in all of the functions of MediaPlayerPrivateGStreamer.cpp. After many tests, I found only in* *MediaPlayerPrivateGStreamer.cpp the statement printf() not always work for me. I don't know why this happen and what's wrong with MediaPlayerPrivateGStreamer.cpp, but anyway, fflush seems to work for me now. Best regards, zqhuan 2012/4/21 Philippe Normand ph...@igalia.com On Fri, 2012-04-20 at 10:42 +0800, ZiQiangHuan wrote: hi， I build webkit with gstreamer support, but I encountered a problem when I test with it. When I write a simple statement printf(* \n) in the function isAvailable() of MediaPlayerPrivateGStreamer.cpp, I got nothing output. When I do the same thing with the function create of MediaPlayerPrivateGStreamer.cpp, I got nothing output too. So I want to know if I want to get some debug info in these functions, what should I do? anyone has some experience with this? And I don't know why this happen. That's odd, the stdout buffer should be flushed since you insert a new line. Try to force a fflush() call maybe? Philippe ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Eliminate potential null pointer dereference?
On Fri, Apr 20, 2012 at 1:48 PM, Rachel Blum gr...@chromium.org wrote: I completely agree with Maciej here that if this is a reachable code, then the patch author should put a reasonable effort into creating a test case. And most importantly, these changes are clearly not code cleanup. I'm disagreeing here. (as far as NULL checks go). Unless there's a demonstrable reason that you _need_ a value uninitialized, why is the burden of proof on the person doing cleanup? Yes, at the point the code was written, it's well possible that the author was aware that the value would always be initialized for use. However, if code is added to a class, that invariant is not always checked again. The burden of proof of correctness is always on the patch author in WebKit. It doesn't matter whether the patch is a simple cleanup, adding a new feature, or fixing a bug. I think the confusion is over the intent of the person making the cleanup change. We (I speak as one of the people pushing static analysis) are not interested in *changing* WebKit behavior. We're interested in making sure behavior is deterministic. Requiring the construction of what amounts to an exploit for each fix for uninitialized variables seems a bit overkill :) I agree that the CHANGELOG entry should state that we deliberately didn't add tests. My personal policy is to propose those patches, complete with No new tests/ cleanup only. If I get pushback on the review, I'm happy to abandon it. As multiple reviewers have repeatedly stated in this thread, I don't think cleanup only is an acceptable description of a change of this nature. cleanup only implies that there is no behavioral change. If we're modifying the code to make WebKit more deterministic, then there is an observable behavior change, namely that WebKit is more deterministic than it used to be. And in this particular case, WebKit may even have one less crash bug. Such a behavioral change should ideally be tested. If it cannot be tested or that coming up with a test case is too hard, then it should be explained in the change log, and I don't consider cleanup as a satisfactory explanation. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
