|
|
Created:
5 years, 8 months ago by Shrikant Kelkar Modified:
5 years, 8 months ago CC:
chromium-reviews, wfh+watch_chromium.org, rickyz+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding checks in sandbox code to get some data on AppContainer based CreateProcess failures.
Due to limited debugging options, given it doesn't repro locally, adding some checks in sandbox code to gather more data.
BUG=467920
R=jschuh@chromium.org,cpu@chromium.org
Committed: https://crrev.com/cb0da85dcf65824f96233ed77fd04d99b4c031cb
Cr-Commit-Position: refs/heads/master@{#325663}
Patch Set 1 #
Total comments: 4
Patch Set 2 : CHECK() replaced with DumpWithoutCrash(). #Messages
Total messages: 17 (2 generated)
wfh@chromium.org changed reviewers: + wfh@chromium.org
https://codereview.chromium.org/1093443002/diff/1/sandbox/win/src/target_proc... File sandbox/win/src/target_process.cc (right): https://codereview.chromium.org/1093443002/diff/1/sandbox/win/src/target_proc... sandbox/win/src/target_process.cc:164: CHECK(FALSE); Have you considered just returning a new SBOX error code here? Then the error should appear in Process.Sandbox.Launch.Error UMA histogram also - is this the same issue as 385714 and 453541, it certainly looks like it from the bug...
https://codereview.chromium.org/1093443002/diff/1/sandbox/win/src/target_proc... File sandbox/win/src/target_process.cc (right): https://codereview.chromium.org/1093443002/diff/1/sandbox/win/src/target_proc... sandbox/win/src/target_process.cc:164: CHECK(FALSE); On 2015/04/15 23:56:29, Will Harris wrote: > Have you considered just returning a new SBOX error code here? Then the error > should appear in Process.Sandbox.Launch.Error UMA histogram > > also - is this the same issue as 385714 and 453541, it certainly looks like it > from the bug... Yes, that's what I was communicating with reporter earlier. I think reporter definitely is stuck with issues 385714, but besides that I am receiving 122 ERROR_INSUFFICIENT_BUFFER which I want to find reason for. This function returns GetLastError codes, sorry but didn't get what you mean by return SBOX error code.
https://codereview.chromium.org/1093443002/diff/1/sandbox/win/src/target_proc... File sandbox/win/src/target_process.cc (right): https://codereview.chromium.org/1093443002/diff/1/sandbox/win/src/target_proc... sandbox/win/src/target_process.cc:164: CHECK(FALSE); On 2015/04/16 00:06:36, Shrikant Kelkar wrote: > On 2015/04/15 23:56:29, Will Harris wrote: > > Have you considered just returning a new SBOX error code here? Then the error > > should appear in Process.Sandbox.Launch.Error UMA histogram > > > > also - is this the same issue as 385714 and 453541, it certainly looks like it > > from the bug... > > Yes, that's what I was communicating with reporter earlier. I think reporter > definitely is stuck with issues 385714, but besides that I am receiving 122 > ERROR_INSUFFICIENT_BUFFER which I want to find reason for. > > This function returns GetLastError codes, sorry but didn't get what you mean by > return SBOX error code. ah okay I meant instead of returning GetLastError you could create six new SBOX_* error codes in sandbox_types.h then return a different one from each potential failure point, in order to know where it's failing, rather than doing a CHECK() which will terminate the browser process. However, this would mean losing the GetLastError() since you can only return one thing - so perhaps just if(::GetLastError() == ERROR_INSUFFICIENT_BUFFER) checks around the new error codes? You could also consider using base::debug::DumpWithoutCrashing() if you want the diagnosis code not to terminate the browser process.
https://codereview.chromium.org/1093443002/diff/1/sandbox/win/src/target_proc... File sandbox/win/src/target_process.cc (right): https://codereview.chromium.org/1093443002/diff/1/sandbox/win/src/target_proc... sandbox/win/src/target_process.cc:164: CHECK(FALSE); On 2015/04/16 00:13:36, Will Harris wrote: > On 2015/04/16 00:06:36, Shrikant Kelkar wrote: > > On 2015/04/15 23:56:29, Will Harris wrote: > > > Have you considered just returning a new SBOX error code here? Then the > error > > > should appear in Process.Sandbox.Launch.Error UMA histogram > > > > > > also - is this the same issue as 385714 and 453541, it certainly looks like > it > > > from the bug... > > > > Yes, that's what I was communicating with reporter earlier. I think reporter > > definitely is stuck with issues 385714, but besides that I am receiving 122 > > ERROR_INSUFFICIENT_BUFFER which I want to find reason for. > > > > This function returns GetLastError codes, sorry but didn't get what you mean > by > > return SBOX error code. > > ah okay I meant instead of returning GetLastError you could create six new > SBOX_* error codes in sandbox_types.h then return a different one from each > potential failure point, in order to know where it's failing, rather than doing > a CHECK() which will terminate the browser process. > > However, this would mean losing the GetLastError() since you can only return one > thing - so perhaps just if(::GetLastError() == ERROR_INSUFFICIENT_BUFFER) checks > around the new error codes? > > You could also consider using base::debug::DumpWithoutCrashing() if you want the > diagnosis code not to terminate the browser process. Basically, I don't want to make these changes permanent in src/sandbox/.. It just get more diagnostic information and then revert, as I am running out of option finding the root cause. DumpWithoutCrashing was the option I tried before, but that was in content/../sandbox_win.cc, the mystery remains which exact function from TargetProcess::Create fails with ERROR_INSUFFICIENT_BUFFER. IIRC cpu/rvargas had a different take on adding DumpWithoutCrashing into src/sandbox/.. (Bringing in dependency). I guess.. if I am going to revert immediately cpu@ may allow this :).
Just to clarify, do we think every tab is crashing for these users today? Because if so, then I can see landing this for a day or two. If not, do we know roughly how many unique users are hitting this crash? Because, I'm worried we're going to turn a small nuisance into a big nuisance.
On 2015/04/16 23:59:39, jschuh (out April 2015) wrote: > Just to clarify, do we think every tab is crashing for these users today? > Because if so, then I can see landing this for a day or two. If not, do we know > roughly how many unique users are hitting this crash? Because, I'm worried we're > going to turn a small nuisance into a big nuisance. For 44.0.2368.0 64-W 7day around 1480 users are reporting this type of error (Not sure out of how many though). Instead of causing actual crash how do you feel about pulling in dependency of DumpWithoutCrash to src/sandbox for couple of days before revert happens. From users who communicated to us, it doesn't seem a consistent repro either, the video one person sent suggests that he has to try multiple times opening/closing tabs before he gets error and even after closing that failed tab he still can continue opening new tabs in same session.
Yeah, I'm not okay with this then because it would turn an occasional sad tab into a full browser crash. I'm far more comfortable with temporarily pulling the dependency and then reverting it in a day or two.
On 2015/04/17 00:29:30, jschuh (out April 2015) wrote: > Yeah, I'm not okay with this then because it would turn an occasional sad tab > into a full browser crash. I'm far more comfortable with temporarily pulling the > dependency and then reverting it in a day or two. Changed CHECK() into DumpWithoutCrash(). ptal.
lgtm. just make sure it gets reverted right after you get the data.
The CQ bit was checked by shrikant@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1093443002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/cb0da85dcf65824f96233ed77fd04d99b4c031cb Cr-Commit-Position: refs/heads/master@{#325663}
Message was sent while issue was closed.
I see 169 reports on canary 44.0.2374.0 so far (top crasher), can I revert this now?
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/1057083006/ by shrikant@chromium.org. The reason for reverting is: Collected dumps, reverting.. |