Closed Bug 1381005 Opened 7 years ago Closed 7 years ago

Crash in libxul.so@0xc242b6 | mozilla::net::nsSocketTransport::SendStatus

Categories

(Core :: Networking: HTTP, defect)

55 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed

People

(Reporter: dragana, Assigned: dragana)

References

Details

(Keywords: crash, Whiteboard: [necko-active])

Crash Data

Attachments

(1 file, 1 obsolete file)

      No description provided.
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Whiteboard: [necko-active]
This is crash in MOZ_DIAGNOSTIC_ASSERT(mEnt) that I have added.

Maybe I was wrong to expect that mEnt must not be 0. I look at that code for so long and I do not see how this is happening. Strangely code later in the same function has check for mEnt != nullptr.
Keywords: crash
(In reply to Dragana Damjanovic [:dragana] from comment #1)
> This is crash in MOZ_DIAGNOSTIC_ASSERT(mEnt) that I have added.
> 
> Maybe I was wrong to expect that mEnt must not be 0. I look at that code for
> so long and I do not see how this is happening. Strangely code later in the
> same function has check for mEnt != nullptr.

gHttpHandler->CoalesceSpdy() can null mEnt (I think)
There is a user comment saying that it happened while watching a youtube video.
Crash Signature: [libxul.so@0xc242b6 | mozilla::net::nsSocketTransport::SendStatus] → [@libxul.so@0xc242b6 | mozilla::net::nsSocketTransport::SendStatus ]
(In reply to Dragana Damjanovic [:dragana] from comment #3)
> There is a user comment saying that it happened while watching a youtube
> video.

I am running nightly on linux playing anything on youtube for 6 hours still no crash :(
Crash Signature: [@libxul.so@0xc242b6 | mozilla::net::nsSocketTransport::SendStatus ] → [@libxul.so@0xc242b6 | mozilla::net::nsSocketTransport::SendStatus ] [@ mozilla::net::nsHttpConnectionMgr::nsHalfOpenSocket::SetupStreams]
Crash Signature: [@libxul.so@0xc242b6 | mozilla::net::nsSocketTransport::SendStatus ] [@ mozilla::net::nsHttpConnectionMgr::nsHalfOpenSocket::SetupStreams] → [@libxul.so@0xc242b6 | mozilla::net::nsSocketTransport::SendStatus ] [@ mozilla::net::nsHttpConnectionMgr::nsHalfOpenSocket::SetupStreams] [@ mozilla::net::nsHttpConnectionMgr::nsHalfOpenSocket::OnOutputStreamReady ]
Crash Signature: [@libxul.so@0xc242b6 | mozilla::net::nsSocketTransport::SendStatus ] [@ mozilla::net::nsHttpConnectionMgr::nsHalfOpenSocket::SetupStreams] [@ mozilla::net::nsHttpConnectionMgr::nsHalfOpenSocket::OnOutputStreamReady ] → [@libxul.so@0xc242b6 | mozilla::net::nsSocketTransport::SendStatus] [@ mozilla::net::nsHttpConnectionMgr::nsHalfOpenSocket::SetupStreams] [@ mozilla::net::nsHttpConnectionMgr::nsHalfOpenSocket::OnOutputStreamReady] [@ mozilla::net::nsHttpConnectionMgr:…
Attached patch bug_find_howw_ment_is_null.patch (obsolete) — Splinter Review
This make nsConnectionEntry ref countable. And adds a lot of MOZ_DIAGNOSTIC_ASSERT to figure out why mEnt gets nulled.

This is only temporary, probably it will run only for a day.

On the other side maybe we can make nsConnectionEntry ref countably in any case, but that will be in a different patch.
Attachment #8887011 - Flags: review?(mcmanus)
Comment on attachment 8887011 [details] [diff] [review]
bug_find_howw_ment_is_null.patch

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

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +1926,5 @@
>      }
>      if (!ci) {
>          ci = mConnInfo;
>      }
> +    MOZ_DIAGNOSTIC_ASSERT(ci);

hope we don't see that one in crash stacks :)

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +3956,5 @@
>          mBackupStreamOut = nullptr;
>      }
>  
>      // Lose references to input stream (and backup).
> +    if (mStreamIn) {

its possible this is helpful for more than diagnostics :)

@@ +4315,5 @@
>          // Restore callbacks.
>          mStreamOut->AsyncWait(this, 0, 0, nullptr);
>          mSocketTransport->SetEventSink(this, nullptr);
>          mSocketTransport->SetSecurityCallbacks(this);
> +        mStreamIn->AsyncWait(nullptr, 0, 0, nullptr);

obv this should have been mstreamin the whole time, right? but should that first param be this?

::: netwerk/protocol/http/nsHttpConnectionMgr.h
@@ +335,5 @@
>              uint32_t maxCount = 0);
>  
>          // Remove the empty pendingQ in |mPendingTransactionTable|.
>          void RemoveEmptyPendingQ();
> +        uint8_t mHowItWasRemoved;

please make this an enum

@@ +408,5 @@
>          void FastOpenNotSupported() override;
>          void SetFastOpenStatus(uint8_t tfoStatus) override;
>          void CancelFastOpenConnection();
> +
> +        RefPtr<nsConnectionEntry>     mEnt;

I originally was going to ask you to keep this stuff private and create one or more assert methods on the class.. but that seems like a big ask.

so how about you move these to their own private section with a big comment (instead of landing in the existing public one) and include the necessary friend references that can later be removed?
Attachment #8887011 - Flags: review?(mcmanus) → review+
Pushed by dd.mozilla@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59c86ba26101
Add some diagnostic assertions to figure out why mEnt is null. This is only temporary. r=mcmanus
https://hg.mozilla.org/integration/mozilla-inbound/rev/59c86ba2610141e0c9fa82defdae58f919a5c6bd
Bug 1381005 - Add some diagnostic assertions to figure out why mEnt is null. This is only temporary. r=mcmanus
Attachment #8887011 - Attachment is obsolete: true
Attachment #8887241 - Flags: review+
Keywords: leave-open
This crash actually went away with this patch. The patch made ConnectioEntry ref counted.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Keywords: leave-open
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: