1744321 - Some Google Ads are broken with Fission after updating to 94.0.2

Status: RESOLVED FIXED

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

Description

[Alice]

Attachment: 2021-12-03 11-05-25.mkv

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/96.0.4664.45 Safari/537.36 Edg/96.0.1054.29

Steps to reproduce:

• Using Firefox 94.0.2
• Visit https://www.popsilla.com/ or https://www.getpaint.net/
• Click ad
• Click pixel will show in networking tab, however nothing from the ad is opened

Actual results:

Ad will fire click pixel in the networking tab but will not actually open the ad.

Expected results:

Firefox should open the ads on click. This issue only exists on certain sites, sites like ESPN or NBC News seem to work.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Networking' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 2

[Valentin Gosu [:valentin]]

The bugs seems to only happen with fission enabled fission.autostart=true.
Moreover, when I run this in a debug build, I trigger this assertion in WindowContext::HasValidTransientUserGestureActivation

#8  mozilla::dom::WindowContext::HasValidTransientUserGestureActivation() (this=<optimized out>) at /docshell/base/WindowContext.cpp:469
#9  0x00007f6da0ec942d in mozilla::dom::BrowsingContext::IsSandboxedFrom(mozilla::dom::BrowsingContext*) (this=0x7f6d589f6400, aTarget=aTarget@entry=0x7f6d5b8c0200) at /docshell/base/BrowsingContext.cpp:1360
#10 0x00007f6da0ecc67d in mozilla::dom::BrowsingContext::CheckSandboxFlags(nsDocShellLoadState*) (this=0x7f6d5b8c0200, aLoadState=0x7f6d5eab2760) at /docshell/base/BrowsingContext.cpp:1936
#11 mozilla::dom::BrowsingContext::InternalLoad(nsDocShellLoadState*) (this=0x7f6d5b8c0200, aLoadState=0x7f6d5eab2760) at /docshell/base/BrowsingContext.cpp:2065
#12 0x00007f6d9f95b6a7 in mozilla::dom::WindowGlobalParent::RecvInternalLoad(nsDocShellLoadState*) (this=0x7f6d5ec35000, aLoadState=0x7f6d5eab2760) at /dom/ipc/WindowGlobalParent.cpp:372
#13 0x00007f6d9d513f0c in mozilla::dom::PWindowGlobalParent::OnMessageReceived(IPC::Message const&) (this=<optimized out>, msg__=...) at PWindowGlobalParent.cpp:960
#14 0x00007f6d9d299fda in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) (this=0x7f6d5b29f400, msg__=...) at PContentParent.cpp:6551
#15 0x00007f6d9d1a0740 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) (this=0x7f6d5b29f4f8, aProxy=0x7f6d6c2ecac0, aMsg=...) at /ipc/glue/MessageChannel.cpp:2043

Comment 3

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

If I disable fission.autostart, I can't reproduce this.

Comment 4

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

It appears as though the cause of this issue is that the navigation is navigating a cross-origin frame, but when the InternalLoad navigation request arrives at the target frame, the HasValidTransientUserGestureActivation bit is not set. We'll probably need to do something to avoid running into this validation case for loads which redirect across process boundaries.

We should make sure we're still doing the relevant checks in the source process after making this change.

Comment 5

[Chris Peterson [:cpeterson]]

TBD whether we will want to uplift this fix to a (hypothetical) 95.0.1 dot release.

Comment 6

[Edgar Chen [:edgar]]

We don't allow access to the user gesture state on an OOP iframe due to various reasons.
For load case, we should use the HasValidTransientUserGestureActivation bit from nsDocShellLoadState instead.

Comment 7

[Edgar Chen [:edgar]]

Attachment: Bug 1744321 - Add IsInProcess() check while checking sandbox flags; r=nika

Comment 8

[Edgar Chen [:edgar]]

Okay, it seems the existing tests only test the same-origin cases, need to figure out how to add tests for cross-origin.

Comment 9

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2cbfdb8a5fcef974a4fdd8ee8c9506d7b07f9cd

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ab20bd3f0eb
Add IsInProcess() check while checking sandbox flags; r=nika

Comment 11

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/8ab20bd3f0eb

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:edgar, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 14

[Edgar Chen [:edgar]]

Comment on attachment 9254088 [details]
Bug 1744321 - Add IsInProcess() check while checking sandbox flags; r=nika

Beta/Release Uplift Approval Request

• User impact if declined: Users can not navigate the page via clicking a link inside an OOP iframe (with Fission enabled).
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change is simple and only affects sandboxing check, I have added a test to ensure the change won't break the in-process iframe case.
• String changes made/needed: None

Comment 16

[Dianna Smith [:diannaS]]

Comment on attachment 9254088 [details]
Bug 1744321 - Add IsInProcess() check while checking sandbox flags; r=nika

Approved for 96.0b5

Comment 17

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/250ef2d6a53c