1908134 - Fix null pointer bug in `PrefWithoutDefault`

Status: NEW

(GeckoView :: General, defect, P2)

Description

[Olivia Hall [:olivia]]

PrefWithoutDefault has a bug that will produce a crash if the value is attempted to get while still null. get().

An example of the stack trace may be found here. Unused patch that reproduces this failure and failing test examples.

[task 2024-03-11T14:51:25.409Z] 14:51:25 INFO - 03-11 14:51:24.809 E/GeckoSessionTestRule( 3449): java.lang.NullPointerException: Attempt to invoke virtual method 'boolean java.lang.Boolean.booleanValue()' on a null object reference

This might be a seperate bug or a part of this one, please split if the scope is too much:

Additionally, paraphrasing :jonalmeida, noted that:
There might also be a related or same root cause bug around RuntimeSettings#runWithAllPrefs.

• PrefWithoutDefault calls commit using super.commit.
• super.commit() then calls addToBundle around here
• The PrefWithoutDefault addToBundle is unused and likely was the intended addToBundle to use.
• So calling RuntimeSettings#runWithAllPrefs might cause a crash.

Comment 1

[[:owlish] 🦉 PST]

Olivia, can you add a bit more info?

Comment 2

[Olivia Hall [:olivia]]

Sure, for anyone interested in taking this as a good first bug, I'd recommend:

• Ideally, PrefWithoutDefault should not have public @Nullable T get() as an option.
  • Recommend first jumping to the testing section and creating a test that reproduces the issue.
  • To fix this, a sensible default could be checking to see if Gecko has a value. (If Gecko does have a value, then this is the acting pref anyway!)
  • When ready to work on the fix:
    • A new message could be sent, such as GeckoView:GetGeckoPref or GeckoView:GetGeckoPrefs in GeckoViewStartup.sys.mjs
    • It could be patterened off of GeckoView:SetDefaultPrefs here. However, instead of setting prefs, we should get the values for use.
  • Then, on the Java side, around here We could call EventDispatcher.getInstance().dispatch("GeckoView:GetGeckoPref", data); and get a value.
  • I'm suspicious it might also be possible to get null or undefined from this API. In that case, if Gecko returns null, then consider just sending a sensible primative type result back, e.g., "", false, or 0.

For testing:

• Recommend looking at existing PrefWithoutDefault usage and basing tests off of that.
• A known way to reproduce the original issue is found here. Unused patch that reproduces this failure and failing test examples. This pref no longer uses PrefWithoutDefault, but a new test could be patterened off of it. I'd recommmend getting a reproducing failure first, before attempting changes.

This would also set the stage for 1888979 as part of that bug would additionally need a clear way to GeckoView:GetGeckoPref.

Comment 3

[Alex Johnson]

I can take this one.

Comment 4

[Olivia Hall [:olivia]]

Thanks for your interest! I assigned the bug to you. Please feel free to needinfo me for questions or for a review.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit BugBot documentation.

Comment 6

[JCesium]

Good Afternoon,
I'm wondering if I can take on this bug as my first bugfix?

Thanks!

Comment 7

[JCesium]

(In reply to JCesium from comment #6)

Good Afternoon,
I'm wondering if I can take on this bug as my first bugfix?

Thanks!

Cancel that actually. I don't have the right setup to really debug and test this appropriately.