1990313 - storage.sync.set with object nested deeper than 128 layers throws (via webext-storage dependency on Rust's serde_json)

Status: NEW

(WebExtensions :: Storage, defect, P3)

Description

[Ben Dean-Kawamura [:bdk]]

https://bugzilla.mozilla.org/show_bug.cgi?id=1989840 describes issues where Firefox crashes after failing to parse JSON. For that bug I'm going to fix the crash, but there will still be parsing issues. I'm not sure exactly what the behavior will be, but I'm guessing it will cause issues for where extensions will fail to persist their data and/or fail to sync. The first step for this bug is to identify steps-to-reproduce so that we can understand the issue better.

Comment 1

[Rob Wu [:robwu]]

Test case is at https://bugzilla.mozilla.org/show_bug.cgi?id=1989840#c17

STR:

• Load extension at about:debugging, attached at https://bugzilla.mozilla.org/show_bug.cgi?id=1989840#c17

Expected:

• Tab opens at https://example.com/OK

Actual:

• Tab opens at https://example.com/FAIL:storage.set()__throws with the error being "An unexpected error occurred" (with no further info due to bug 1990529).
• (actual from Firefox 135 until 144: crash due to bug 1989840)

Comment 2

[Rob Wu [:robwu]]

Edited title for clarity based on the only known case where valid JSON is rejected.

• With the patch from bug 1990529, I can see that the original error is "recursion limit exceeded", which could originate from https://searchfox.org/firefox-main/rev/9140dac1eab834541a75792a78a5ed2fbee39142/third_party/rust/serde_json/src/error.rs#384
• RecursionLimitExceeded described at https://searchfox.org/firefox-main/rev/9140dac1eab834541a75792a78a5ed2fbee39142/third_party/rust/serde_json/src/error.rs#309-310
• set at https://searchfox.org/firefox-main/rev/9140dac1eab834541a75792a78a5ed2fbee39142/third_party/rust/serde_json/src/de.rs#1372-1375
• if_checking_recursion_limit is defined at https://searchfox.org/firefox-main/rev/9140dac1eab834541a75792a78a5ed2fbee39142/third_party/rust/serde_json/src/de.rs#1362,1364
• that flag is set if disable_recursion_limit() is called, according to https://searchfox.org/firefox-main/rev/9140dac1eab834541a75792a78a5ed2fbee39142/third_party/rust/serde_json/src/de.rs#170-216 and documented at https://lib.rs/crates/serde_json/features#feature-unbounded_depth

Is it feasible to use the disable_recursion_limit feature? If not, a work-around could be to serialize outside Rust, but then we'd double-serialize, which may be unfortunate. On the other hand, doing so could also fix part of bug 1960449 (see ACTION REQUIRED for storage API at https://bugzilla.mozilla.org/show_bug.cgi?id=1960449#c8).

Comment 3

[Ben Dean-Kawamura [:bdk]]

I made https://bugzilla.mozilla.org/show_bug.cgi?id=1991095 with some ideas on how to fix this. I'd like to avoid disable_recursion_limit since it opens the door to some other nasty crashes (like stack overflows). I think we need to investigate more to know the solution, but my guess is that the avoid the depth at the browser level option would be the best. I think it might require an extra level of serialization, but I'm hoping any performance hit will be negligible.