Prevent crash when a null or open GeckoSession is registered with GeckoView.setSession()

RESOLVED FIXED in Firefox 60

Status

defect
RESOLVED FIXED
Last year
7 months ago

People

(Reporter: rbarker, Assigned: rbarker)

Tracking

Trunk
mozilla60

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment)

GeckoView will either crash if a null GeckoSession is set or throw an exception if the GeckoSession is open.
Assignee: nobody → rbarker
Comment on attachment 8956281 [details]
Bug 1443283 - Prevent crash when a null or open GeckoSession is registered with GeckoView.setSession()

https://reviewboard.mozilla.org/r/225170/#review231374

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoView.java
(Diff revision 1)
>          }
>      }
>  
>      public void setSession(final GeckoSession session) {
> -        if (mSession != null && mSession.isOpen()) {
> -            throw new IllegalStateException("Current session is open");

We still need to throw here. Otherwise we'd leak an open session.
Attachment #8956281 - Flags: review?(nchen) → review-
It's only a leak if GeckoView owns the session which won't be the case with tabs or in the case of the VR browser.
GeckoView doesn't know if the consumer is taking ownership of the session, and I don't think a naive caller of `setSession` would assume it has to take ownership of the existing session first.

I'd prefer leaving this check in-place, and add a `releaseSession()` call that bypasses this check, and returns the released session.
Comment on attachment 8956281 [details]
Bug 1443283 - Prevent crash when a null or open GeckoSession is registered with GeckoView.setSession()

https://reviewboard.mozilla.org/r/225170/#review231804

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoView.java:195
(Diff revision 2)
>          if (mSurfaceView != null) {
>              mSurfaceView.setBackgroundColor(color);
>          }
>      }
>  
> +    public void releaseSession() {

`releaseSession` should return the released session
Attachment #8956281 - Flags: review?(nchen) → review+
(In reply to Jim Chen [:jchen] [:darchons] from comment #6)
>
> `releaseSession` should return the released session

I had it that way at originally, but figured if someone wants the current session they should just call getSession first?
(In reply to Jim Chen [:jchen] [:darchons] from comment #6)
> Comment on attachment 8956281 [details]
> Bug 1443283 - Prevent crash when a null or open GeckoSession is registered
> with GeckoView.setSession()
> 
> https://reviewboard.mozilla.org/r/225170/#review231804
> 
> :::
> mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoView.java:
> 195
> (Diff revision 2)
> >          if (mSurfaceView != null) {
> >              mSurfaceView.setBackgroundColor(color);
> >          }
> >      }
> >  
> > +    public void releaseSession() {
> 
> `releaseSession` should return the released session

I like returning the session because it signifies that ownership of the session is being transferred to the caller.
Comment on attachment 8956281 [details]
Bug 1443283 - Prevent crash when a null or open GeckoSession is registered with GeckoView.setSession()

https://reviewboard.mozilla.org/r/225170/#review231866

LGTM with releaseSession returning the session.
Attachment #8956281 - Flags: review?(esawin) → review+
Pushed by rbarker@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a824b10df47
Prevent crash when a null or open GeckoSession is registered with GeckoView.setSession() r=jchen,esawin
https://hg.mozilla.org/mozilla-central/rev/9a824b10df47
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 60 → mozilla60
You need to log in before you can comment on or make changes to this bug.