TimelineView breaks when set model() is called before ready()

catapult-project / catapult

Deprecated Catapult GitHub. Please instead use http://crbug.com "Speed>Benchmarks" component for bugs and https://chromium.googlesource.com/catapult for downloading and editing source code..

https://chromium.googlesource.com/catapult

BSD 3-Clause "New" or "Revised" License

1.93k stars 563 forks source link

TimelineView breaks when set model() is called before ready() #2252

Open natduca opened 8 years ago

natduca commented 8 years ago

We assume in the TimelineView that model will not be called until after ready(). That breaks under very quirky circumstances.

Repro:

$catapult/tracing/bin/trace2html tracing/test_data/big_trace.json
go to http://localhost:8003/tracing/test_data/big_trace.html
reload it, but switch to another tab while its reloading (use a hotkey, you have to move fast)
return to that tab. the tab will not have loaded.

scheibo commented 8 years ago

FWIW, internally I ended up using a hacky workaround (cl/119996942) where I just added:

if(!this.trackViewContainer_){this.domReady();}

as the first line of https://github.com/catapult-project/catapult/blob/master/tracing/tracing/ui/timeline_view.html#L360.

would obviously be better to get the ordering right to begin with, but this at least "works"

anniesullie commented 8 years ago

@zeptonaut @Apeliotes since they're working on polymer 1.0 conversion and ordering issues can affect that.

scheibo commented 8 years ago

Hi, any updates here? This is still an issue (and my previous hack no longer works as the code has changed :P). It seems like the error messages are slightly different, attached is a screenshot of what the console looks like when you try to open up the trace viewer in a background tab. error

natduca commented 8 years ago

Hey @scheibo sorry I dropped the ball on this. I think I can get to this sometime this week.

natduca commented 8 years ago

Working on this now.

natduca commented 8 years ago

I am still debugging but this may take a lot longer than I had hoped...

Sadly, the problems you're now seeing have been exacerbated by the Polymer1.0 conversion @anniesullie mentioned in the may comment up above.

@zeptonaut @Apeliotes I think the problem here root causes in #2580, which is causing different tracing situations load polymer in shadow mode while others use shady mode. And, for some reason the shady dom version of trace2html breaks very specifically as described above. I have no clue why yet.

I started trying to fix #2580, but then the d3 hack here added to finish the conversion (no bug filed :/) reared its ugly head: https://github.com/catapult-project/catapult/blob/master/tracing/tracing/ui/base/base.html#L14.

We must go deeper.

On the bright side, I think I have a straightforward fix to the original problem stated in this bug. But, I can't test it because of the new issues from the polymer1 conversion.

natduca commented 8 years ago

I have a CL up for fixing #2580 (https://codereview.chromium.org/2267503002) and in so doing unearthed #2698. On the bright side, that seems to fix this bug completely, including the repro that used to trigger your original issue.

It shouldn't fix this because I know the underlying issue is valid. But, it is a timing bug so now my old repro probably just doesn't trigger it. Sigh!

I did put out a CL that I'm pretty sure fixes the original bug, which I will land speculatively since its not a "bad" patch in itself: https://codereview.chromium.org/2266703002

@scheibo would you mind seeing if you can repro with ALL THE THINGS applied? ;)