1485492 - Disable fastblocking when the user has interacted with a document

Status: RESOLVED FIXED

(Firefox :: Protections UI, enhancement, P2)

Description

[(no longer active)]

We had an email discussion about what to do with post-load XHRs and the like with FastBlock.  Right now, they would all be blocked.  But this can introduce problems such as blocking XHRs that things like video players depend on, which could result in breakage.

It is probably a good idea to exclude documents that the user has interacted with (those where nsIDocument::UserHasInteracted() returns true for) from FastBlock.  The easiest way to do this is probably to add a loadgroup flag to disable fastblocking based on, set that flag to true from nsIDocument::SetUserHasInteracted(), and check it in Necko when deciding whether to activate fastblock...

Comment 1

[Johann Hofmann [:johannh]]

Just to make sure I understand correctly, Necko would check/respect this flag when the request _starts_, right (as opposed to checking the flag when the request is potentially cancelled)? I would assume most documents are interacted with once after at least 5s.

Comment 2

[(no longer active)]

(In reply to Johann Hofmann [:johannh] from comment #1)
> Just to make sure I understand correctly, Necko would check/respect this
> flag when the request _starts_, right (as opposed to checking the flag when
> the request is potentially cancelled)?

Those two are the same time.  The requests are cancelled right when they are started, if fastblock kicks in.  If at the time the request is started, we're not 5 seconds past the navigation start time, fastblock won't blocked anything.

> I would assume most documents are
> interacted with once after at least 5s.

That is probably true!  :-)

Comment 3

[Andrea Marchesini [:baku]]

Attachment: userInteraction.patch

Comment 4

[(no longer active)]

Comment on attachment 9005199 [details] [diff] [review]
userInteraction.patch

Review of attachment 9005199 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDocument.cpp
@@ +12608,5 @@
>    }
> +
> +  nsCOMPtr<nsILoadInfo> loadInfo = mChannel ? mChannel->GetLoadInfo() : nullptr;
> +  if (loadInfo) {
> +    loadInfo->SetDocumentHasUserInteracted(true);

Can you please do this right after setting mUserHasInteracted above, in case some day MaybeAllowStorageForOpener() starts to open a channel? :-)

And maybe also do a sanity check assert that the boolean on the loadinfo is false before you set it?

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +700,5 @@
>  
> +    // If the user has interacted with the document, we disable fastblock.
> +    if (mLoadInfo && mLoadInfo->GetDocumentHasUserInteracted()) {
> +        return false;
> +    }

Good, but let's check the pref *first*.  IOW it seems better if you appended a condition to the end of this check: https://searchfox.org/mozilla-central/rev/05d91d3e02a0780f44599371005591d7988e2809/netwerk/protocol/http/nsHttpChannel.cpp#712

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b26f3da46d8
Disable fastblocking when the user has interacted with a document, r=ehsan

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/3b26f3da46d8