Bug 82310 – Handle DRT crash caused by Android OOM.

• Component: New Bugs
• Status: RESOLVED FIXED

[Hao Zheng]

Handle DRT crash caused by Android OOM.

[Hao Zheng]

Created attachment 134003 [details]
Patch

[Adam Barth]

Comment on attachment 134003 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134003&action=review

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:580
> +        if crash and line is not None:
> +            error.append(line)

I'm not sure I quite understand what these lines do.  Can you explain a bit more?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:484
> +            if drt_return == 137:

Can we name this constant?

[Adam Barth]

This looks fine, I'm just unsure about the change to chromium.py.

[Hao Zheng]

(In reply to comment #2)
> (From update of attachment 134003 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134003&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:580
> > +        if crash and line is not None:
> > +            error.append(line)
> 
> I'm not sure I quite understand what these lines do.  Can you explain a bit more?
> 

Sure. This is to append the last output line when crash happens.

> > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:484
> > +            if drt_return == 137:
> 
> Can we name this constant?

Will replace with signal.SIGKILL.

[Adam Barth]

Comment on attachment 134003 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134003&action=review

Ok.

>>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:580
>>> +            error.append(line)
>> 
>> I'm not sure I quite understand what these lines do.  Can you explain a bit more?
> 
> Sure. This is to append the last output line when crash happens.

I see.  Previously we were truncating.

[Hao Zheng]

Created attachment 134009 [details]
Patch

[Tony Chang]

Comment on attachment 134009 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134009&action=review

Should there be test cases for this?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:551
> +        return None if (return_match is None) else int(return_match.group(1))

Nit: This could be "return return_match and int(return_match.group(1))".

[WebKit Review Bot]

Comment on attachment 134009 [details]
Patch

Rejecting attachment 134009 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Hao Zheng]

(In reply to comment #7)
> (From update of attachment 134009 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134009&action=review
> 
> Should there be test cases for this?
> 

As it involves adb command and a recursion of 10 levels, I could not find a good way to test it. Any suggestion on this? I could add tests in https://bugs.webkit.org/show_bug.cgi?id=80852 .

> > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:551
> > +        return None if (return_match is None) else int(return_match.group(1))
> 
> Nit: This could be "return return_match and int(return_match.group(1))".

Ternary operator looks more readable to me. Is there any style guideline on this?

Tony, as I don't have commit right, could you set commit-queue for me? Thanks!

[Tony Chang]

(In reply to comment #9)
> (In reply to comment #7)
> > (From update of attachment 134009 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=134009&action=review
> > 
> > Should there be test cases for this?
> > 
> 
> As it involves adb command and a recursion of 10 levels, I could not find a good way to test it. Any suggestion on this? I could add tests in https://bugs.webkit.org/show_bug.cgi?id=80852 .

Can you mock out adb?

> > > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:551
> > > +        return None if (return_match is None) else int(return_match.group(1))
> > 
> > Nit: This could be "return return_match and int(return_match.group(1))".
> 
> Ternary operator looks more readable to me. Is there any style guideline on this?

No style guide, hence a nit.

[Hao Zheng]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #7)
> > > (From update of attachment 134009 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=134009&action=review
> > > 
> > > Should there be test cases for this?
> > > 
> > 
> > As it involves adb command and a recursion of 10 levels, I could not find a good way to test it. Any suggestion on this? I could add tests in https://bugs.webkit.org/show_bug.cgi?id=80852 .
> 
> Can you mock out adb?
> 

Good idea. Need some refactoring. Will do in bug 80852.

> > > > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:551
> > > > +        return None if (return_match is None) else int(return_match.group(1))
> > > 
> > > Nit: This could be "return return_match and int(return_match.group(1))".
> > 
> > Ternary operator looks more readable to me. Is there any style guideline on this?
> 
> No style guide, hence a nit.

[WebKit Review Bot]

Comment on attachment 134009 [details]
Patch

Clearing flags on attachment: 134009

Committed r112359: <http://trac.webkit.org/changeset/112359>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.