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

Status: RESOLVED FIXED

(GeckoView :: General, defect)

Description

[Randall Barker [:rbarker]]

GeckoView will either crash if a null GeckoSession is set or throw an exception if the GeckoSession is open.

Comment 1

[Randall Barker [:rbarker]]

Attachment: Bug 1443283 - Prevent crash when a null or open GeckoSession is registered with GeckoView.setSession()

Review commit: https://reviewboard.mozilla.org/r/225170/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/225170/

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 3

[Randall Barker [:rbarker]]

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.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

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 5

[Randall Barker [:rbarker]]

Comment on attachment 8956281 [details]
Bug 1443283 - Prevent crash when a null or open GeckoSession is registered with GeckoView.setSession()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225170/diff/1-2/

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

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

Comment 7

[Randall Barker [:rbarker]]

(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?

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

(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 9

[Eugen Sawin [:esawin]]

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.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/9a824b10df47