1613431 - Figure out how synced fields should work if the owner (Browsing/WindowContext) is discarded

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P1)

Description

[Olli Pettay [:smaug][[e-mail]]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1612894#c1

Comment 1

[Nika Layzell [:nika] (ni? for response)]

The current behaviour here is "If the target has been discarded, changes will be ignored", rather than the crashing behaviour we previously had. https://searchfox.org/mozilla-central/rev/5a10be606f2d76ef22f1f44565749490de991d35/docshell/base/SyncedContext.h#48

In terms of behaviours which we could have here, I think there are 3 options:

1. Crash. This is the old behaviour, and we stopped doing it because people wouldn't find these situations when testing, and we'd end up introducing crashes for users.
2. Ignore the request silently. This is close(-ish) to the current behaviour. We technically return a nsresult which is failing if the set failed, but nobody checks it.
3. Mark the methods as MUST_USE and return a nsresult, or similar type, requiring callers to either Unused << the value, or deal with the case where the BC is discarded.

ni?s to a variety of people in case they have thoughts here.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

I like 3. That at least forces one to think whether it matters if syncing failed.

Comment 3

[Andreas Farre [:farre]]

I think I agree with Olli. Number 3.

Comment 4

[Chris Peterson [:cpeterson]]

kmag votes for option 3.

M5 to prevent introducing new regressions.

Comment 5

[Chris Peterson [:cpeterson]]

M6 because we've now decided what to do, so it doesn't need to block M5. Should be a high priority for M6!

Comment 6

[Chris Peterson [:cpeterson]]

Tentatively moving P1 Fission M6 bugs to M6a.

Comment 7

[Neha Kochar [:neha]]

Andreas, would you be able to work on this to implement #3 as mentioned by Nika in comment 1?

Comment 8

[Andreas Farre [:farre]]

Attachment: Bug 1613431 - Part 1: Have synced setters return nsresult. r=nika

Comment 9

[Andreas Farre [:farre]]

Attachment: Bug 1613431 - Part 2: Ignore synced setters return value. r=nika

Depends on D83645

Comment 10

[Andreas Farre [:farre]]

Attachment: Bug 1613431 - Part 3: Handle synced setters return value. r=nika

Depends on D83646

Comment 11

[Andreas Farre [:farre]]

Attachment: Bug 1613431 - Part 4: Require that synced field setters' return value is handled. r=nika

Depends on D83647

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d29677fb419
Part 1: Have synced setters return nsresult. r=nika
https://hg.mozilla.org/integration/autoland/rev/b326d33f98cb
Part 2: Ignore synced setters return value. r=nika
https://hg.mozilla.org/integration/autoland/rev/de69e3c1635a
Part 3: Handle synced setters return value. r=nika
https://hg.mozilla.org/integration/autoland/rev/2698c61b00f5
Part 4: Require that synced field setters' return value is handled. r=nika

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4d29677fb419
https://hg.mozilla.org/mozilla-central/rev/b326d33f98cb
https://hg.mozilla.org/mozilla-central/rev/de69e3c1635a
https://hg.mozilla.org/mozilla-central/rev/2698c61b00f5