1671571 - Deprecated loadUrl() API not backwards compatible (null headers)

Status: RESOLVED FIXED

(GeckoView :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

This time I tried to just suppress the deprecation warnings to unblock the update, but we started to see a crash in the UI tests:

java.lang.NullPointerException: Attempt to invoke interface method 'int java.util.Map.size()' on a null object reference

     FATAL EXCEPTION: main
Process: org.mozilla.samples.browser, PID: 6199
java.lang.NullPointerException: Attempt to invoke interface method 'int java.util.Map.size()' on a null object reference
	at org.mozilla.geckoview.GeckoSession$Loader.additionalHeaders(GeckoSession.java:1646)
	at org.mozilla.geckoview.GeckoSession.loadUri(GeckoSession.java:1870)
	at mozilla.components.browser.engine.gecko.GeckoEngineSession.loadUrl(GeckoEngineSession.kt:149)
	at mozilla.components.concept.engine.EngineSession.loadUrl$default(EngineSession.kt:546)
	at mozilla.components.browser.session.engine.middleware.LinkingMiddleware$performLoadOnMainThread$1.invokeSuspend(LinkingMiddleware.kt:82)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:56)
	at android.os.Handler.handleCallback(Handler.java:739)
	at android.os.Handler.dispatchMessage(Handler.java:95)
	at android.os.Looper.loop(Looper.java:135)
	at android.app.ActivityThread.main(ActivityThread.java:5221)
	at java.lang.reflect.Method.invoke(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:372)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:899)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:694)

• https://github.com/mozilla-mobile/android-components/issues/8715
• https://console.firebase.google.com/project/moz-android-components-230120/testlab/histories/bh.9f526cd30412cc12/matrices/7562499020072772908/executions/bs.4353a10d54e156c8
• https://treeherder.mozilla.org/#/jobs?repo=android-components&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&revision=5cd371eebc9e2dfef8cc45472dd52a72b5a36558

It looks the previous version had no issues with null headers and now we get a NullPointerException.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

I may switch to the new API in AC instead of waiting for a fix in GV - but filed this bug anyways in case we want to fix this in GeckoView too, to be fully backwards compatible.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Switching to the new API was straight-forward. But updating out tests is seems complicated: Bug 1671578

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Due to what I mentioned in Bug 1671578, for now I decided to just map null to an empty map on the AC side before passing it to GeckoView.

Comment 4

[[ex-Mozilla] Agi Sferro | :agi]

Well we made a new API so there's no backward compatibility problems involved by definition :)

In general there's no reason to accept a null header (before it was to reduce the numbers of overloads, but with the builder pattern we don't have that problem). And the method is marked as @NonNull so I'm surprised you can even pass null in there in Kotlin.

I'm WONTFIX-ing this, but please reopen if you still think we should support this.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Well, it's @Nullable in loadUri() and then you pass it to additionalHeaders():
https://searchfox.org/mozilla-central/rev/e0eb861a187f0bb6d994228f2e0e49b2c9ee455e/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java#1767-1772

The backward compatibility that broke is that existing code using loadUri() just starts to crash now if they have null headers, which didn't happen before.

Comment 6

[[ex-Mozilla] Agi Sferro | :agi]

Ooh I see! Yeah that should be fixed, I'll send a patch soon.

Comment 7

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1671571 - Don't pass null headers in Loader.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/100f7a3b25f8
Don't pass null headers in Loader. r=snorp

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/100f7a3b25f8