Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1930783002:
Fix more android logging crashes in run-webkit-tests. (Closed)
|
Created:
4 years, 7 months ago by Dirk Pranke Modified:
4 years, 7 months ago CC:
chromium-reviews, blink-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix more android logging crashes in run-webkit-tests.
This CL attempts to fix yet more places where run-webkit-tests
can crash when running tests on Android because we get confused
over whether the byte stream coming from the device is unicode
or not.
R=jbudorick@chromium.org, mgiuca@chromium.org, mithro@chromium.org, robertshield@chromium.org
BUG=598449
Committed: https://crrev.com/ddb327c1485aa354b0e7702933510f739f32d2a2
Cr-Commit-Position: refs/heads/master@{#390299}
Patch Set 1 #
Messages
Total messages: 25 (9 generated)
The CQ bit was checked by dpranke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1930783002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1930783002/1
Description was changed from ========== Fix more android logging crashes in run-webkit-tests. This CL attempts to fix yet more places where run-webkit-tests can crash when running tests on Android because we get confused over whether the byte stream coming from the device is unicode or not. R=jbudorick@chromium.org, mgiuca@chromium.org BUG=598449 ========== to ========== Fix more android logging crashes in run-webkit-tests. This CL attempts to fix yet more places where run-webkit-tests can crash when running tests on Android because we get confused over whether the byte stream coming from the device is unicode or not. R=jbudorick@chromium.org, mgiuca@chromium.org, mithro@chromium.org BUG=598449 ==========
dpranke@chromium.org changed reviewers: + tansell@chromium.org
Description was changed from ========== Fix more android logging crashes in run-webkit-tests. This CL attempts to fix yet more places where run-webkit-tests can crash when running tests on Android because we get confused over whether the byte stream coming from the device is unicode or not. R=jbudorick@chromium.org, mgiuca@chromium.org, mithro@chromium.org BUG=598449 ========== to ========== Fix more android logging crashes in run-webkit-tests. This CL attempts to fix yet more places where run-webkit-tests can crash when running tests on Android because we get confused over whether the byte stream coming from the device is unicode or not. R=jbudorick@chromium.org, mgiuca@chromium.org, mithro@chromium.org, robertshield@chromium.org BUG=598449 ==========
dpranke@chromium.org changed reviewers: + robertshield@chromium.org
First one to stamp wins :) ...
lgtm
On 2016/04/28 03:09:07, robertshield wrote: > lgtm darn lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/04/28 03:10:37, jbudorick wrote: > On 2016/04/28 03:09:07, robertshield wrote: > > lgtm > > darn Heh, by 60 seconds. I'll let you know if the prize is awesome. > > lgtm
On 2016/04/28 at 03:16:45, robertshield wrote:
> On 2016/04/28 03:10:37, jbudorick wrote:
> > On 2016/04/28 03:09:07, robertshield wrote:
> > > lgtm
> >
> > darn
>
> Heh, by 60 seconds. I'll let you know if the prize is awesome.
>
> >
> > lgtm
This change feels like a hack to work around a bigger problem -- what is the
actual error you are coming across?
It sounds like there are a couple of things wrong here;
* Code isn't decoding incoming data into unicode objects.
* Code is using slices on byte strings, effectively mangling utf-8.
* The place you are logging to doesn't have the correct encoding settings.
For example, doing the following will break if the data had a multibyte
character around position 79.
a[:80].decode('utf-8')
This confusion about str and unicode is the primary reason for Python 3.
Tim 'mithro' Ansell
PS It's tansell@chromium.org - I'll look into getting mithro@chromium.org as I
do have mithro@google.com....
On 2016/04/28 03:23:01, mithro wrote:
> On 2016/04/28 at 03:16:45, robertshield wrote:
> > On 2016/04/28 03:10:37, jbudorick wrote:
> > > On 2016/04/28 03:09:07, robertshield wrote:
> > > > lgtm
> > >
> > > darn
> >
> > Heh, by 60 seconds. I'll let you know if the prize is awesome.
> >
> > >
> > > lgtm
>
> This change feels like a hack to work around a bigger problem -- what is the
> actual error you are coming across?
The linked bug contains a link to one failing build w/ this crash. You can look
at the builder and see that we're crashing the test harness every few runs.
> It sounds like there are a couple of things wrong here;
> * Code isn't decoding incoming data into unicode objects.
This is the primary problem. Different parts of the android.py port
talk to things on the device different ways, and those different communications
return unpredictable responses. Some (to me unknown) set of them return
arbitrary byte strings. Some, I believe, can actually return text.
The executive.run_command() defaults to trying to decode text into
utf-8, which doesn't work (hence changing the decode_output flag).
However, we also don't want to log arbitrary strings in inconsisent
encodings, so this CL tries to convert things into a known, consistent
encoding as close to the edge of the system as possible (which
is consistent with good Python unicode practice).
> * Code is using slices on byte strings, effectively mangling utf-8.
I'm not so worried about this one.
> * The place you are logging to doesn't have the correct encoding settings.
I'm not sure quite what you mean by this, but the answer is either "yes"
or "the logging destination should be getting input in a consistent format,
not encoding stuff itself", depending on how you look at it.
> For example, doing the following will break if the data had a multibyte
> character around position 79.
> a[:80].decode('utf-8')
That will break, but decode('utf-8', errors='replace') won't.
> This confusion about str and unicode is the primary reason for Python 3.
It's certainly one of the main reasons, yes.
The CQ bit was checked by dpranke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1930783002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1930783002/1
Message was sent while issue was closed.
Description was changed from ========== Fix more android logging crashes in run-webkit-tests. This CL attempts to fix yet more places where run-webkit-tests can crash when running tests on Android because we get confused over whether the byte stream coming from the device is unicode or not. R=jbudorick@chromium.org, mgiuca@chromium.org, mithro@chromium.org, robertshield@chromium.org BUG=598449 ========== to ========== Fix more android logging crashes in run-webkit-tests. This CL attempts to fix yet more places where run-webkit-tests can crash when running tests on Android because we get confused over whether the byte stream coming from the device is unicode or not. R=jbudorick@chromium.org, mgiuca@chromium.org, mithro@chromium.org, robertshield@chromium.org BUG=598449 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
On 2016/04/28 at 04:40:32, commit-bot wrote: > Committed patchset #1 (id:1) Why was the correct fix not the one shown here -> https://codereview.chromium.org/1924183002
Message was sent while issue was closed.
On 2016/04/28 04:54:10, mithro wrote: > On 2016/04/28 at 04:40:32, commit-bot wrote: > > Committed patchset #1 (id:1) > > Why was the correct fix not the one shown here -> > https://codereview.chromium.org/1924183002 That is probably the better fix; it probably would've caught: https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Android%20%28N... which my change didn't. Feel free to revert my change and land yours instead. If one was paranoid, you could check to see if there were any callers of run_command that might care whether the string had corrected errors in it, but that seems unlikely, or whether it was important in some place that the errors raised exceptions rather than being replaced and effectively ignored.
Message was sent while issue was closed.
On 2016/04/28 at 06:40:53, dpranke wrote: > On 2016/04/28 04:54:10, mithro wrote: > > On 2016/04/28 at 04:40:32, commit-bot wrote: > > > Committed patchset #1 (id:1) > > > > Why was the correct fix not the one shown here -> > > https://codereview.chromium.org/1924183002 > > That is probably the better fix; it probably would've caught: > > https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Android%20%28N... > > which my change didn't. > > Feel free to revert my change and land yours instead. > > If one was paranoid, you could check to see if there were any callers of run_command that > might care whether the string had corrected errors in it, but that seems unlikely, or > whether it was important in some place that the errors raised exceptions rather than > being replaced and effectively ignored. If you LGTM mine I'll revert this one and land mine.
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/1925083002/ by tansell@chromium.org. The reason for reverting is: Reverting to land https://codereview.chromium.org/1924183002 instead..
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/ddb327c1485aa354b0e7702933510f739f32d2a2 Cr-Commit-Position: refs/heads/master@{#390299}
Message was sent while issue was closed.
Description was changed from ========== Fix more android logging crashes in run-webkit-tests. This CL attempts to fix yet more places where run-webkit-tests can crash when running tests on Android because we get confused over whether the byte stream coming from the device is unicode or not. R=jbudorick@chromium.org, mgiuca@chromium.org, mithro@chromium.org, robertshield@chromium.org BUG=598449 ========== to ========== Fix more android logging crashes in run-webkit-tests. This CL attempts to fix yet more places where run-webkit-tests can crash when running tests on Android because we get confused over whether the byte stream coming from the device is unicode or not. R=jbudorick@chromium.org, mgiuca@chromium.org, mithro@chromium.org, robertshield@chromium.org BUG=598449 Committed: https://crrev.com/ddb327c1485aa354b0e7702933510f739f32d2a2 Cr-Commit-Position: refs/heads/master@{#390299} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
