1829666 - mozilla::dom::SessionHistoryEntry::GetWindowState crashes when accessed from the parent, breaking devtools printouts (MOZ_CRASH(This lives in the child process))

Status: NEW

(Core :: DOM: Navigation, task, P5)

Description

[:Gijs (he/him)]

STR:

1. open browser console or browser debugger
2. type gBrowser.selectedBrowser.browsingContext and hit enter. So far so good.
3. arrow up, add .activeSessionHistoryEntry, hit enter. So far so good.
4. click the expando arrow next to the .activeSessionHistoryEntry

ER:
print something meaningful in the console

AR:
crash

(alternative expected results: Don't expose crashy things to devtools as enumerable properties.)

Stack points to https://searchfox.org/mozilla-central/rev/26790fecfcda622dab234b28859da721b80f3a35/docshell/shistory/SessionHistoryEntry.cpp#684 and otherwise has JS (presumably from devtools) near the top.

Comment 1

[Andreas Farre [:farre] (PTO -> Aug 1st)]

Peter, is there an easy fix to this?

Comment 2

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

Is there a reason we'd want to fix this? The assertion is there on purpose.
And once non-SHIP is gone, we can remove the relevant code.

Comment 3

[:Gijs (he/him)]

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #2)

Is there a reason we'd want to fix this?

I'd like console logging things not to crash the browser. That doesn't seem particularly unreasonable.

The assertion is there on purpose.

Can you elaborate what that purpose is, and if it could also be served by only returning an error code (or null or w/e) from these getters? And/or could we only crash in automation?

Comment 4

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

The purpose is to not use that property ever in the parent process.
I guess we could also just throw, but the relevant code should be going away reasonable soon, so not sure it is worth to tweak anything here. That crash has been there after all for couple of years now. But if this is blocking something, throwing an exception normally and crashing in automation sounds good.
(Is throwing an exception ok for devtools usage?)

Comment 5

[:Gijs (he/him)]

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #4)

The purpose is to not use that property ever in the parent process.
I guess we could also just throw, but the relevant code should be going away reasonable soon, so not sure it is worth to tweak anything here. That crash has been there after all for couple of years now. But if this is blocking something,

It's not, so if this is going away soon enough (is doing so not blocked on android fission?) then making this P5 and marking it as dependent on the bug removing this stuff seems reasonable enough.

throwing an exception normally and crashing in automation sounds good.
(Is throwing an exception ok for devtools usage?)

Assuming this means returning non-NS_OK which throws in JS, then yes that should be fine.

Comment 6

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #4)

The purpose is to not use that property ever in the parent process.
I guess we could also just throw, but the relevant code should be going away reasonable soon,

Adam, I wonder if the removal of the relevant code should be incorporated into your wips https://bugzilla.mozilla.org/show_bug.cgi?id=1892551 or if it makes more sense to have another bug tracking. Either way, please help set the bug dependency properly.

so not sure it is worth to tweak anything here. That crash has been there after all for couple of years now. But if this is blocking something, throwing an exception normally and crashing in automation sounds good.
(Is throwing an exception ok for devtools usage?)

Comment 7

[Adam Vandolder [:avandolder]]

Since this property becomes unused when the non-SHIP implementation is removed, it does seem reasonable to add it to my patches, yeah.