758972 - Is Spdy VerifyStream() needed?

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Description

[Patrick McManus [:mcmanus]]

bug 753663 tracked a crash where a invalid spdy stream was used.. generally under the signature mozilla::net::SpdyStream::UpdateTransportReadEvents 

the source of the crash has never been fully understood.

bug 756551 fixed that crash through a series of integrity checks and theoretically un-needed intializations.

when we have a full nightly window we should change VerifyStream() to be DEBUG only and see if the crashes have been genuinely fixed or not via nightly crash stats. (we can expect about 1 a day on nightly.. 4+ or so with a current sized aurora audience).

Comment 1

[Patrick McManus [:mcmanus]]

I believe 759438 was the underlying cause of the corruption.

Given that we have a fresh nightly cycle I think we should turn VerifyStream() into a DEBUG-build only feature and look for regressions in crash stats.

Comment 2

[Patrick McManus [:mcmanus]]

Attachment: patch 0

Comment 3

[Honza Bambas (:mayhemer)]

Comment on attachment 643879 [details] [diff] [review]
patch 0

Review of attachment 643879 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/SpdySession3.cpp
@@ +704,5 @@
> +#ifndef DEBUG
> +  // Only do the real verification in debug builds
> +  return true;
> +#endif
> +  fprintf(stderr,"VERIFY\n");

fprintf leftover?

Comment 4

[Patrick McManus [:mcmanus]]

(In reply to Honza Bambas (:mayhemer) from comment #3)

> 
> fprintf leftover?

doh! thanks.

Comment 5

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/43f259b15ccc

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/43f259b15ccc