Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1104193003:
cc: Test BeginMainFrame values come from BeginImplFrame. (Closed)
|
Created:
5 years, 7 months ago by mithro-old Modified:
5 years, 2 months ago CC:
chromium-reviews, cc-bugs_chromium.org, zhaoqin1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncc: Test BeginMainFrame values come from BeginImplFrame.
The frame time in the BeginFrameArgs sent to BeginMainFrame should match a
value from a BeginImplFrame. However there is not a 1:1 relantionship, when the
main thread is slow, there many be multiple impl frames.
DEPS=http://crrev.com/787763006
BUG=346230
R=brianderson,enne
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/3785ebac79741ebc1428c9c50a8cd4bc92f0e924
Cr-Commit-Position: refs/heads/master@{#331933}
Committed: https://crrev.com/71ee0851476d87b5c2d8121a196f11ebc1783265
Cr-Commit-Position: refs/heads/master@{#350535}
Patch Set 1 #Patch Set 2 : Rebase onto master. #Patch Set 3 : Rebase onto master. #Patch Set 4 : Rebase onto master #Patch Set 5 : Rebase onto master. #Patch Set 6 : Rebase onto master. #Patch Set 7 : Don't use references. #Messages
Total messages: 31 (10 generated)
Hi Brian / Enne, Here is the test I promised to upload and related to https://codereview.chromium.org/1099703004/ This test is a a very strict test which makes sure that the main thread and impl thread are using the same concept of frame time. This test however can't be landed until the LTHI::Now() fix is landed. The test should catch the problem found in https://codereview.chromium.org/1099703004/ because in the CompositeImmediately path the generated BeginFrameArgs were never sent to the impl thread. Getting this patch to pass reliably in *all* cases has been what I have been one of the major things I have been trying to achieve for the last ~12 months. Tim 'mithro' Ansell
lgtm
The CQ bit was checked by mithro@mithis.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1104193003/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mithro@mithis.com
The patchset sent to the CQ was uploaded after l-g-t-m from brianderson@chromium.org Link to the patchset: https://codereview.chromium.org/1104193003/#ps20001 (title: "Rebase onto master.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1104193003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/a003f9bc651a572fea3881cfb72a7d8842c62192 Cr-Commit-Position: refs/heads/master@{#328929}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/1133763002/ by ksakamoto@chromium.org. The reason for reverting is: LayerTreeHostTestBeginMainFrameTimeIsAlsoImplTime started failing since a dependent patch (https://codereview.chromium.org/787763006/) is reverted. example: https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests/builds/24421 .
The CQ bit was checked by mithro@mithis.com
The patchset sent to the CQ was uploaded after l-g-t-m from brianderson@chromium.org Link to the patchset: https://codereview.chromium.org/1104193003/#ps40001 (title: "Rebase onto master.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1104193003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)
The CQ bit was checked by mithro@mithis.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1104193003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/3785ebac79741ebc1428c9c50a8cd4bc92f0e924 Cr-Commit-Position: refs/heads/master@{#331933}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/1162763002/ by zhaoqin@google.com. The reason for reverting is: Dr.Memory bot report use-after-free error on newly added test. BUG=493852.
Hi zhaoqin, Can you please link to something that shows the DrMemory bot reporting the failure? It is very hard to find the issue after coming back to it 2-3 days later. I can't see how this test itself is doing a use after free, so it is likely this is pointing to a more serious issue. Also any idea why this didn't also turn up on the *san bots? Tim 'mithro' Ansell
On 2015/06/01 03:58:56, mithro wrote: > Hi zhaoqin, > > Can you please link to something that shows the DrMemory bot reporting the > failure? It is very hard to find the issue after coming back to it 2-3 days > later. I put issue number in the revert CL: crbug.com/493852 > > I can't see how this test itself is doing a use after free, so it is likely this > is pointing to a more serious issue. > > Also any idea why this didn't also turn up on the *san bots? I am not 100% sure, but usually two reasons: 1. Dr.Memory's slowdown expose some racy free/reference bug 2. Windows only bug (*san bots are Linux only) > > Tim 'mithro' Ansell
On 2015/06/01 04:14:44, zhaoqin wrote: > On 2015/06/01 03:58:56, mithro wrote: > > Hi zhaoqin, > > > > Can you please link to something that shows the DrMemory bot reporting the > > failure? It is very hard to find the issue after coming back to it 2-3 days > > later. > > I put issue number in the revert CL: crbug.com/493852 Ah sorry, it appears you have multiple BUG= lines so I missed the first one you linked too. > > > > I can't see how this test itself is doing a use after free, so it is likely > this > > is pointing to a more serious issue. > > > > Also any idea why this didn't also turn up on the *san bots? > > I am not 100% sure, but usually two reasons: > 1. Dr.Memory's slowdown expose some racy free/reference bug > 2. Windows only bug (*san bots are Linux only) Are there any trybots which run Dr.Memory so I can replicate the result and figure out the cause? Looking at https://www.chromium.org/developers/how-tos/using-drmemory doesn't seem to reveal anything... The information in crbug.com/493852 does look like this test is discovering a real issue inside the LTHI code. It might also be the root cause of the https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre... Tim 'mithro' Ansell
On Mon, Jun 1, 2015 at 12:24 AM, <mithro@mithis.com> wrote: > On 2015/06/01 04:14:44, zhaoqin wrote: > >> On 2015/06/01 03:58:56, mithro wrote: >> > Hi zhaoqin, >> > >> > Can you please link to something that shows the DrMemory bot reporting >> the >> > failure? It is very hard to find the issue after coming back to it 2-3 >> days >> > later. >> > > I put issue number in the revert CL: crbug.com/493852 >> > > Ah sorry, it appears you have multiple BUG= lines so I missed the first > one you > linked too. > > > >> > I can't see how this test itself is doing a use after free, so it is >> likely >> this >> > is pointing to a more serious issue. >> > >> > Also any idea why this didn't also turn up on the *san bots? >> > > I am not 100% sure, but usually two reasons: >> 1. Dr.Memory's slowdown expose some racy free/reference bug >> 2. Windows only bug (*san bots are Linux only) >> > > Are there any trybots which run Dr.Memory so I can replicate the result and > figure out the cause? Looking at > https://www.chromium.org/developers/how-tos/using-drmemory doesn't seem to > reveal anything... > Sorry no, but it should be easy to run the test under Dr.Memory if you have a Windows machine. You can run test under Dr.Memory directly without recompile anything. The different compilation option is only for better call-stack. > > The information in crbug.com/493852 does look like this test is > discovering a > real issue inside the LTHI code. It might also be the root cause of the > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre... > > > Tim 'mithro' Ansell > > https://codereview.chromium.org/1104193003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by mithro@mithis.com
The patchset sent to the CQ was uploaded after l-g-t-m from brianderson@chromium.org Link to the patchset: https://codereview.chromium.org/1104193003/#ps120001 (title: "Don't use references.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1104193003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1104193003/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/71ee0851476d87b5c2d8121a196f11ebc1783265 Cr-Commit-Position: refs/heads/master@{#350535} |
