1420334 - Intermittent any test PROCESS-CRASH | application crashed [@ mozilla::TimeStamp::operator-(mozilla::TimeStamp const &)] after Assertion failure: !aOther.IsNull() (Cannot compute with aOther null value)

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect, P3)

Description

[Treeherder Bug Filer]

Filed by: philringnalda [at] gmail.com

https://treeherder.mozilla.org/logviewer.html#?job_id=147295335&repo=autoland

https://queue.taskcluster.net/v1/task/Dkka1gH8QIG6GMEA_PcQQw/runs/0/artifacts/public/logs/live_backing.log

Comment 4

[Junior [inactive]]

OnStartRequest is in the main thread, but OnCacheEntryCheck depends on the open thread.
Sometimes we encounter race condition in mOnStartRequestTimestamp.

Since this is only for telemetry, I'd like to add a null-check here instead of lock.

Comment 5

[Junior [inactive]]

Still suffer from no Atomic<mozilla::TimeStamp>.
Consider to use Atmoic<PRTime>.

Comment 6

[Intermittent Failures Robot]

15 failures in 744 pushes (0.02 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* autoland: 6
* mozilla-inbound: 5
* mozilla-central: 4

Platform breakdown:
* osx-10-10: 9
* windows10-64: 3
* windows7-32: 1
* linux32: 1
* android-4-3-armv7-api16: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1420334&startday=2017-11-20&endday=2017-11-26&tree=all

Comment 7

[Michal Novotny [:michal]]

(In reply to Junior[:junior] from comment #4)
> OnStartRequest is in the main thread, but OnCacheEntryCheck depends on the
> open thread.
> Sometimes we encounter race condition in mOnStartRequestTimestamp.
> 
> Since this is only for telemetry, I'd like to add a null-check here instead
> of lock.

We already use mRCWNLock in nsHttpChannel::OnCacheEntryCheck, so why not to simply grab the lock at https://searchfox.org/mozilla-central/rev/7a8c667bdd2a4a32746c9862356e199627c0896d/netwerk/protocol/http/nsHttpChannel.cpp#6945 ?

Comment 8

[Junior [inactive]]

Since lock is not free.
However, we can lock only if the network wins.

Thanks michal!

Comment 9

[Junior [inactive]]

Attachment: lockTimestamp, v1

A simple approach

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebd5716a625db57b84994f35f9e4d64a581e9664

Comment 10

[Michal Novotny [:michal]]

Comment on attachment 8933208 [details] [diff] [review]
lockTimestamp, v1

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

Looks good. Maybe add a comment explaining why the lock is needed.

Comment 11

[Junior [inactive]]

Attachment: lockTimestamp, v2

add comment and carry r+

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f6112300d6e
protect timestamp setting in a critical section, r=michal

Comment 13

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/1f6112300d6e

Comment 14

[Intermittent Failures Robot]

33 failures in 792 pushes (0.042 failures/push) were associated with this bug in the last 7 days. 

This is the #28 most frequent failure this week.  

** This failure happened more than 30 times this week! Resolving this bug is a high priority. **

** Try to resolve this bug as soon as possible. If unresolved for 2 weeks, the affected test(s) may be disabled. **  

Repository breakdown:
* autoland: 19
* mozilla-inbound: 14

Platform breakdown:
* osx-10-10: 13
* windows7-32: 8
* windows10-64: 7
* linux64: 5

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1420334&startday=2017-11-27&endday=2017-12-03&tree=all