1646532 - Fix GetInProcessTopInternal usage in nsGlobalWindowOuter::ShouldPromptToBlockDialogs/AreDialogsEnabled/EnableDialogs/DisableDialogs

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Comment 1

[Kris Maglione [:kmag]]

Also goes for nsGlobalWindowOuter::TemporarilyDisableDialogs

Comment 2

[Kris Maglione [:kmag]]

And nsGlobalWindowInner::DialogsAreBeingAbused

Comment 3

[Neha Kochar [:neha]]

Seems to be related to alert()-spam. With current implementation, would not impact oop subframes.

Comment 4

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

I think we should be able to do this by moving topWindow->mAreDialogsEnabled onto the toplevel WindowContext as a synced field, as we only ever clear it to false outside of nsIDOMWindowUtils::EnableDialogs, which can probably be deleted as I don't see any callers in-tree (https://bugzilla.mozilla.org/show_bug.cgi?id=1669392)

The mDialogAbuseCount flag might be nice to support across processes, but that may be trickier. We would probably need to start tracking the information in the parent process on WindowGlobalParent, and move when we're rejecting things to a different process in the dialog flow.

Comment 5

[Neha Kochar [:neha]]

Doesn't have to block M7, let's fix in M7a.

Comment 6

[Neha Kochar [:neha]]

Johann, do you have thoughts for mDialogAbuseCount (see comment 4)?

Comment 7

[Neha Kochar [:neha]]

Ethan recommended to ask pbz to look into this.

Comment 8

[Ethan Tseng [:ethan]]

(In reply to Neha Kochar [:neha] from comment #6)

Johann, do you have thoughts for mDialogAbuseCount (see comment 4)?

Neha, we don't quite understand what you were asking. Could you elaborate your question?

In the meantime, I am redirecting the needinfo to Jens because this code doesn't fit our team's domain expertise.

Comment 9

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

I looked into this a bit more with :neha earlier today, and came to the conclusion that these members generally shouldn't be synced between processes, and should continue to be process-local. There is one use-case which might be useful to be process synced, which is setting mAreDialogsEnabled to false in the background thumbnailer code (https://searchfox.org/mozilla-central/rev/f351e19360729b351bfc7c1386d6e4ca4ea676e2/toolkit/actors/BackgroundThumbnailsChild.jsm#97), however that code is no longer being actively used by the newtab page, and will probably be disabled soon (bug 1706449). It seems fairly reasonable to leave mDialogAbuseCount alone for now, as it will still serve its purpose just fine within a process.

The one change which might be worth making here as a clean-up for fission would be to move these two attributes off of nsGlobalWindowInner and instead put them on BrowsingContextGroup, so that they're shared between all in-process documents within a BrowsingContextGroup, rather than only by in-process docShell subtrees. This should involve no functional changes, mostly just moving the attributes and replacing GetInProcessTopInternal() uses with GetBrowsingContext()->Group(). No cross-process syncing should be necessary :-)

Comment 10

[Neha Kochar [:neha]]

Anny, would you be able to finish up the remaining clean-up here?

Comment 11

[Anny Gakhokidze [:annyG]]

Attachment: Bug 1646532 - Move mAreDialogsEnabled, mLastDialogQuitTime and mDialogAbuseCount to BC group, r=nika!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/990ee1a69537
Move mAreDialogsEnabled, mLastDialogQuitTime and mDialogAbuseCount to BC group, r=nika

Comment 13

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/990ee1a69537