I just had a quick check, and my initial conclusion is that one changed code line is in the crash stack, but too deep -- at #40 frame.

The CL changed content/renderer/render_frame_impl.cc of the crash revision at line 5513-5531 and 5533.

From the stack trace, the closest crashed line is content/renderer/render_frame_impl.cc:5530, but it is in the #40 frame.
For the top 7 frames Findit analyzed, the closest crashed line is content/renderer/render_frame_impl.cc:5621
But it is still too far away. We have a cap of 50 as maximum distance.


In this case, render_frame_impl.cc shows up in top 7 frames quite a few times, while render_frame_impl.cc:5530 is in #40 frame.
We might want to check if this could turn into a rule to filter out CLs.

Assign to Sharu for further investigation.

Hmm, that explanation doesn't make sense to me.  My change affected RenderFrameImpl::NavigateIntgernal, which is frame #0 of the crash stack:

==1==ERROR: AddressSanitizer: heap-use-after-free on address 0x61800015fca8 at pc 0x0000092ef72e bp 0x7fffd43877b0 sp 0x7fffd43877a8
READ of size 8 at 0x61800015fca8 thread T0 (content_shell)
SCARINESS: 51 (8-byte-read-heap-use-after-free)
    #0 0x92ef72d in content::RenderFrameImpl::NavigateInternal(content::CommonNavigationParams const&, content::StartNavigationParams const&, content::RequestNavigationParams const&, std::__1::unique_ptr<content::StreamOverrideParameters, std::__1::default_delete<content::StreamOverrideParameters> >) content/renderer/render_frame_impl.cc:5621:10


The #40 frame that you mention is in the "freed" stack, rather than the "heap-use-after-free" stack.  Is ClusterFuzz not considering the "heap-use-after-free" stack for blame analysis?

Findit does check "heap-use-after-free" content/renderer/render_frame_impl.cc:5621 (as explained above).
But line 5621 is too far away from changed lines 5513-5531 or 5533. The distance is 80+ lines.

However, to avoid false positive, Findit set a maximum distance of 50 lines. Thus this frame was filtered out and we missed this case.
We could increase this distance cap, but we might have to tolerate some false positives.

Wren or Sharu, would you mind trying out the new test script to see what the results from increasing the distance limit would look like? This seems like the perfect opportunity to try it out.

The previous values are essentially just guesses, but we can get a much better idea of what this would affect now.

Actually, according to the maximum distance filtering, I am thinking whether the 50 is a good number, it can just be over-fitting to the data set we collect, I can use the delta test to experiment and find a better number

Comment 3: I see, thanks.  I wonder if it's worth considering anything in the same function, vs number of lines?  While not all cases happen in functions as long as NavigateInternal, it's still better not to have an incentive to avoid explanatory comments, etc.

Something in the same function would probably be ideal, at least in most cases, though it may cause us to miss others. We've discussed this before but didn't have a good way to know that the code was in the same function at the time. It should be possible, but would need a larger refactoring. Either way, these are the types of potential improvements we want to consider now.

There is a easy potential improvement, we can go deeper in the stack and see if there is frame which crashes on the same function in top 7 frames, in this case, we can check the crashed line, if it's within the maximum line distance, we can think the cl is the suspected cl.

Tested 100 max distance, it looks better than the 50, the impact is as below:
https://docs.google.com/a/google.com/spreadsheets/d/1FoPIgXvZp5BmBLvYHc-Y6iIo3eOv62gFZk91o4JBN-U/edit?usp=sharing

The change cl is: https://chromereviews.googleplex.com/511227015/

I'd mark this as fixed, since such case is very rare, and after delta experiment, 100 seems a good value for the maximum distance, and with this maximum distance, the culprit can be found in this case.