1381331 - Crash in mozilla::net::HttpBackgroundChannelParent::OnStartRequestSent

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-2d4822f5-dc39-4faf-8234-6643f0170716.
=============================================================

Crashes started using 20170715030206: http://bit.ly/2tu8CS7

ni on dragana as likely the TCP Fastopen experiment.

Comment 1

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

This assertion seems to be complaining about mBackgroundThread member variable.
This nsCOMPtr is assigned at HttpBackgroundChannelParent ctor on background thread and used in OnXXX functions on main thread.
Could be a thread synchronization issue I introduced for PBg-Http.

Comment 2

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Bug 1381331 - ensure thread-safe access to HttpBackgroundChannelParent::mBackgroundThread.

mBackgroundThread is an nsCOMPtr write on PBackground thread but read on main thread. We need to use
Mutex to ensure memory sync between multiple threads.

Review commit: https://reviewboard.mozilla.org/r/157722/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/157722/

Comment 3

[Honza Bambas (:mayhemer)]

Comment on attachment 8886961 [details]
Bug 1381331 - ensure thread-safe access to HttpBackgroundChannelParent::mBackgroundThread.

https://reviewboard.mozilla.org/r/157722/#review163012

We never release mBackgroundThread during the object's lifetime.  The problem is clearly elsewhere, either mBackgroundThread is assigned null (which is tho impossible) or there is a heap corruption (TFO could be the culprit if this falls in to Dragana's test frame window).  I don't believe this is a memory ordering problem too, ctor has an imlpicit function boundary protection.

Comment 4

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

@mayhemer, thanks for the information. Didn't know that ctor has an implicit function boundary protection.

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 8886961 [details]
Bug 1381331 - ensure thread-safe access to HttpBackgroundChannelParent::mBackgroundThread.

sc, I think I misled you here...

A function barrier is about compile time protection against compiler memory reordering (writes in a function body are made before the function exits), but there is no implicit CPU cache synchronization flush, hence there is no guarantee the writes are observable by other threads.  The ++ performed on the thread's refcnt is a sequentially consistent write (implying release/acquire ordering), but there is no sequentially consistent read (=acquire) performed by the other thread before we touch your object's member.  Hence, there is likely no cache sync performed in the CPU and there might be a need for having a lock here.

Still, I think the cause of this crash is somewhere else.

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

@mayhemer, I've file bug 1382103 since my patch looks legit. We can keep this bug open and close it if crash stopped after my patch landed.

Comment 7

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

All the similar HttpBackgroundChannelParent crash report stopped at 0721 nightly build and bug 1382103 is included in 0722 nightly build. It's highly likely that my analysis in comment #1 is the root cause.
Keep monitoring these crash signatures for few more days before I close these bugs.

Comment 8

[Jim Mathies [:jimm]]

fixed as of 7-21