Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2315213002:
Temporary: Detect WebSocket destruction order problem (Closed)
DescriptionTemporary: Detect WebSocket destruction order problem
Add a CHECK() to crash the browser if a destruction order problem is
encountered.
This will be reverted once the information is collected.
BUG=641013
Committed: https://crrev.com/23169a0f5421e6759d89b1feaa83b06b197a7039
Cr-Commit-Position: refs/heads/master@{#417522}
Patch Set 1 #
Total comments: 2
Patch Set 2 : s/October 2017/October 2016/ #
Messages
Total messages: 26 (13 generated)
The CQ bit was checked by ricea@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Detect WebSocket destruction order problem Add a CHECK() to crash the browser if a destruction order problem is encountered. BUG=641013 ========== to ========== Temporary: Detect WebSocket destruction order problem Add a CHECK() to crash the browser if a destruction order problem is encountered. This will be reverted once the information is collected. BUG=641013 ==========
ricea@chromium.org changed reviewers: + yhirano@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2315213002/diff/1/net/http/http_network_sessi... File net/http/http_network_session.h (right): https://codereview.chromium.org/2315213002/diff/1/net/http/http_network_sessi... net/http/http_network_session.h:324: // TODO(ricea): Remove this by October 2017. Do you mean Oct 2016? Ditto for all occurrences.
The CQ bit was checked by ricea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/2315213002/#ps20001 (title: "s/October 2017/October 2016/")
https://codereview.chromium.org/2315213002/diff/1/net/http/http_network_sessi... File net/http/http_network_session.h (right): https://codereview.chromium.org/2315213002/diff/1/net/http/http_network_sessi... net/http/http_network_session.h:324: // TODO(ricea): Remove this by October 2017. On 2016/09/08 02:27:13, yhirano wrote: > Do you mean Oct 2016? > Ditto for all occurrences. Wow. Okay. Yes. I meant 2016. Fixed.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
ricea@chromium.org changed reviewers: + mmenke@chromium.org
LGTM. What information do you want to get from this? Just whether this occurs? I can't see anything in the crash stacks themselves being too useful.
On 2016/09/08 14:38:42, mmenke wrote: > LGTM. What information do you want to get from this? Just whether this occurs? > I can't see anything in the crash stacks themselves being too useful. My hope is that this is a common crash on an uncommon code path. In this scenario the crash stack would tell me where to look. If it is an uncommon crash on a common code path then the crash stack is only useful in telling me that fact. Since you have handled a lot of URLRequest lifetime bugs, I expect your intuition will be a lot better than mine.
The CQ bit was checked by ricea@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Temporary: Detect WebSocket destruction order problem Add a CHECK() to crash the browser if a destruction order problem is encountered. This will be reverted once the information is collected. BUG=641013 ========== to ========== Temporary: Detect WebSocket destruction order problem Add a CHECK() to crash the browser if a destruction order problem is encountered. This will be reverted once the information is collected. BUG=641013 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Temporary: Detect WebSocket destruction order problem Add a CHECK() to crash the browser if a destruction order problem is encountered. This will be reverted once the information is collected. BUG=641013 ========== to ========== Temporary: Detect WebSocket destruction order problem Add a CHECK() to crash the browser if a destruction order problem is encountered. This will be reverted once the information is collected. BUG=641013 Committed: https://crrev.com/23169a0f5421e6759d89b1feaa83b06b197a7039 Cr-Commit-Position: refs/heads/master@{#417522} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/23169a0f5421e6759d89b1feaa83b06b197a7039 Cr-Commit-Position: refs/heads/master@{#417522}
Message was sent while issue was closed.
On 2016/09/09 06:36:48, Adam Rice wrote: > On 2016/09/08 14:38:42, mmenke wrote: > > LGTM. What information do you want to get from this? Just whether this > occurs? > > I can't see anything in the crash stacks themselves being too useful. > > My hope is that this is a common crash on an uncommon code path. In this > scenario > the crash stack would tell me where to look. If it is an uncommon crash on a > common > code path then the crash stack is only useful in telling me that fact. > > Since you have handled a lot of URLRequest lifetime bugs, I expect your > intuition > will be a lot better than mine. We're just going to see the teardown of ProfileIOData on the stack, most likely the destruction of the bits of the main URLRequestContext (Which means all WebContents have already been destroyed). I suppose we may see a marginally different stack if this only happens when isolated apps are being torn down, but I rather doubt that. Anyhow, not saying I oppose trying this, happy to defer to you on what to try, just skeptical this will get us anything.
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2340253002/ by ricea@chromium.org. The reason for reverting is: We have learnt everything we are going to learn from this CHECK. Reverting as intended. This does not need to be relanded.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
