1842936 - Inconsistency between WindowGlobalParent::IsCurrentGlobal() and nsPIDOMWindowInner::IsCurrentInnerWindow()

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Andrew McCreight [:mccr8]]

nsPIDOMWindowInner::IsCurrentInnerWindow() returns false if SHIP is enabled and the inner window has a BC that is in the BFCache. On the other hand, WindowGlobalParent::IsCurrentGlobal() doesn't consider that condition. This extra condition was added bug 1704068.

An example of where this can happen is in the test browser_contentWindow.js. This test loads one page, then uses BrowserTestUtils.loadURIString() to load a second page, then sends a message to the child actor in the first page. At that point in time, the inner window for the first page returns false for IsCurrentInnerWindow() because the BC is in the bfcache, but IsCurrentGlobal() called on the corresponding WindowGlobalParent returns true.

IsCurrentGlobal() doesn't have many callers, so I'm not sure how bad this is. Should it be changed to add the additional condition?

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Bug 1842936 - Make WindowGlobalParent::IsCurrentGlobal() consistent with nsPIDOMWindowInner::IsCurrentInnerWindow().

Comment 2

[Andrew McCreight [:mccr8]]

The fix is simple but I don't know if it makes sense or not to do.

Comment 3

[Jens Stutte [:jstutte]]

(In reply to Andrew McCreight [:mccr8] from comment #2)

The fix is simple but I don't know if it makes sense or not to do.

:smaug, do you know?

Comment 4

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

Looks like WindowGlobalParent::IsCurrentGlobal() is used very rarely, and callers should be fine with checking bfcache - except that I don't know about the extension code.

Comment 5

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

zombie might know about https://searchfox.org/mozilla-central/search?q=isCurrentGlobal&path=extensions&case=false&regexp=false
At least one of those already explicitly checks for bfcache.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c8336f0ad9fb
Make WindowGlobalParent::IsCurrentGlobal() consistent with nsPIDOMWindowInner::IsCurrentInnerWindow(). r=smaug

Comment 7

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/c8336f0ad9fb

Comment 8

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]: needed for bug 1863852.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: Bug 1842936 - Make WindowGlobalParent::IsCurrentGlobal() consistent with nsPIDOMWindowInner::IsCurrentInnerWindow().

Original Revision: https://phabricator.services.mozilla.com/D183311

Comment 10

[Phabricator Automation]

Uplift Approval Request

• Steps to reproduce for manual QE testing: none
• Explanation of risk level: minor change to a corner case that has been on m-c since September without issue
• Code covered by automated testing: yes
• Is Android affected?: yes
• String changes made/needed: none
• Needs manual QE test: no
• Risk associated with taking this patch: low
• Fix verified in Nightly: yes
• User impact if declined: needed for bug 1863852

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9366098 [details]
Bug 1842936 - Make WindowGlobalParent::IsCurrentGlobal() consistent with nsPIDOMWindowInner::IsCurrentInnerWindow().

Approved for 115.6esr.

Comment 12

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/18785059daf5