Bug 91245

Summary: webkit_unit_test CCLayerTreeHostImplTest.testRemoveRenderPasses started failing.
Product: WebKit Reporter: Vsevolod Vlasov <vsevik>
Component: Tools / TestsAssignee: Dana Jansens <danakj>
Status: RESOLVED FIXED    
Severity: Normal CC: danakj, enne
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch enne: review+

Description Vsevolod Vlasov 2012-07-13 07:44:59 PDT
CCLayerTreeHostImplTest.testRemoveRenderPasses fails on Chromium Linux build.
Probably caused by: http://trac.webkit.org/changeset/122525
See: http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux%2032/builds/20930
Comment 1 Vsevolod Vlasov 2012-07-13 07:49:00 PDT
Skipped: http://trac.webkit.org/changeset/122576
Comment 2 Dana Jansens 2012-07-13 08:49:34 PDT
A better bot backtrace:

	base::debug::StackTrace::StackTrace() [0x7fc4cb993592]
	base::(anonymous namespace)::StackDumpSignalHandler() [0x7fc4cb9f85a5]
	0x7fc4c4929af0
	WTF::OwnPtr<>::operator->() [0x7fc4cde270a0]
	(anonymous namespace)::configureRenderPassTestData() [0x7fc4cde1644d]
	(anonymous namespace)::CCLayerTreeHostImplTest_testRemoveRenderPasses_Test::TestBody() [0x7fc4cde16abc]
Comment 3 Dana Jansens 2012-07-13 08:49:45 PDT
Created attachment 152270 [details]
Patch

The test won't crash for me locally. >_<

From examing the code, this is the only iffy thing I can see. Do you think it would be a good condidate to fix the problem, enne?
Comment 4 Adrienne Walker 2012-07-13 08:53:47 PDT
(In reply to comment #3)
> Created an attachment (id=152270) [details]
> Patch
> 
> The test won't crash for me locally. >_<
> 
> From examing the code, this is the only iffy thing I can see. Do you think it would be a good condidate to fix the problem, enne?

That really doesn't look like it should be null.

Is this a debug vs. release issue? Does a try job crash for you, and can you try the patch there?
Comment 5 Dana Jansens 2012-07-13 10:04:50 PDT
Idk what's up with the trybots but I am unable to apply this change. Even if I remove everything except the "fix". It seems like the trybots are maybe sitting at some version prior to the breaking even occuring.

Either way, I think this change is needed, as it is valid for the compiler to release() the pointer before we deref it for the id().
Comment 6 Adrienne Walker 2012-07-13 10:06:48 PDT
Comment on attachment 152270 [details]
Patch

R=me.  I can buy that argument that renderPass could be null here if the args are evaluated right-to-left.  Let's give this a try.  Please watch the bots when you land this.
Comment 7 Dana Jansens 2012-07-13 10:29:23 PDT
Committed r122602: <http://trac.webkit.org/changeset/122602>
Comment 8 Dana Jansens 2012-07-13 10:55:50 PDT
Looks like the bots went green :D