Closed Bug 1474450 Opened Last year Closed Last year

Make GeckoResult rethrow unhandled exceptions

Categories

(GeckoView :: General, defect, P1)

59 Branch
defect

Tracking

(firefox-esr52 wontfix, firefox-esr60 wontfix, firefox61 wontfix, firefox62 fixed, firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed
firefox63 --- fixed

People

(Reporter: snorp, Assigned: jchen)

Details

(Whiteboard: [geckoview:klar:p1])

Attachments

(3 files)

Currently if you throw an exception in a method that is called from a GeckoResult listener, the exception is caught and the GeckoResult returned from that listener is completed with the exception. If there nobody is listening to that GeckoResult, the exception is dropped on the floor. That's probably not what we want in that scenario most of the time.
Assignee: nobody → nchen
Status: NEW → ASSIGNED
Priority: -- → P1
Comment on attachment 8991074 [details]
Bug 1474450 - 3. Fix javadoc;

https://reviewboard.mozilla.org/r/256070/#review263106
Attachment #8991074 - Flags: review?(nchen) → review+
Comment on attachment 8991072 [details]
Bug 1474450 - 1. Add some tests for more GeckoResult behavior;

https://reviewboard.mozilla.org/r/256066/#review263142
Attachment #8991072 - Flags: review?(snorp) → review+
Comment on attachment 8991073 [details]
Bug 1474450 - 2. Rethrow uncaught and unhandled exceptions in GeckoResult;

https://reviewboard.mozilla.org/r/256068/#review263146

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:153
(Diff revision 1)
> -                    if (valueListener != null && haveValue()) {
> -                        result.completeFrom(valueListener.onValue(mValue));
> -                    } else if (exceptionListener != null && haveError()) {
> +                    if (haveValue()) {
> +                        result.completeFrom(valueListener != null ? valueListener.onValue(mValue)
> +                                                                  : null);
> +                    } else if (!haveError()) {
> +                        // Listener called without completion?
> +                        throw new AssertionError();

We use IllegalStateException elsewhere for this, might make sense here too.
Attachment #8991073 - Flags: review?(snorp) → review+
Comment on attachment 8991073 [details]
Bug 1474450 - 2. Rethrow uncaught and unhandled exceptions in GeckoResult;

https://reviewboard.mozilla.org/r/256068/#review263146

> We use IllegalStateException elsewhere for this, might make sense here too.

I think `AssertionError` makes more sense for cases that should never happen by-design (versus `IllegalStateException` for cases that could happen due to bad input, etc.).
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9c3aeaeee7ec
1. Add some tests for more GeckoResult behavior; r=snorp
https://hg.mozilla.org/integration/autoland/rev/8370b58d0b3b
2. Rethrow uncaught and unhandled exceptions in GeckoResult; r=snorp
https://hg.mozilla.org/integration/autoland/rev/a627429326ab
3. Fix javadoc; r=jchen
[geckoview:klar:p1] because this is a Focus+GV blocker. We should uplift this fix to 62 beta.
Whiteboard: [geckoview:klar:p1]
Comment on attachment 8991072 [details]
Bug 1474450 - 1. Add some tests for more GeckoResult behavior;

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1468048
[User impact if declined]: Incorrect geckoview behavior
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: Bug 1468048
[Is the change risky?]: No
[Why is the change risky/not risky?]: GeckoView-only change.
[String changes made/needed]: None
Attachment #8991072 - Flags: approval-mozilla-beta?
Comment on attachment 8991072 [details]
Bug 1474450 - 1. Add some tests for more GeckoResult behavior;

Geckoview-only issue, let's uplift for beta 9.
Attachment #8991072 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Firefox for Android → GeckoView
Version: Firefox 59 → 59 Branch
Target Milestone: Firefox 63 → mozilla63
You need to log in before you can comment on or make changes to this bug.