1584931 - Disable flashblock list and tests when Fission is enabled

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, task, P1)

Comment 1

[Dimi Lee [:dimi]]

These flashblock test cases open cross-origin iframes and use BrowserTestUtils.browserLoaded to ensure an iframe is loaded.
BrowserTestUtils.browserLoaded requires frame script content-utils.js to be loaded so it can receive load events.
After enabling fission, these test cases fail because BrowserTestUtils.browserLoaded cannot receive a load event when a cross-origin iframe is loaded. This is because that content-utils.js is not imported for the cross-origin iframe’s process.

Here is the investigation result of why frame scripts are not loaded for a cross-origin iframe:
In the normal code flow, when a new process is created by the parent process, frame scripts are loaded in ReallyLoadFrameScrips(). While loading a cross-origin iframe in a child process, the child process asks the parent process to create a new process and ReallyLoadFrameScripts() is not called in this scenario, so the cross-origin doesn’t load any frame script.

Hi Kris,
I am not familiar with the architecture here, so I am asking for help to see if this makes sense to you and if you have any suggestion on how we can fix this. Is this something we should fix inside the test case? or should we make sure a frame script is loaded when a cross-origin iframe is loaded?
Thanks for your help!

Comment 2

[Kris Maglione [:kmag]]

Bug 1578465 will fix browserLoaded for cases where a cross-process navigation needs to occur.

As for frame scripts, they're not intended to be loaded for cross-origin frames. Anything that currently relies on frame scripts for such things needs to be migrated to JSWindowActors.

Comment 3

[Dimi Lee [:dimi]]

Attachment: Bug 1584931 - Replace ContentTask.spawn with SpecialPowers.spawn for flash blocking testcases. r?gcp

To make us pass flash blocking testcases when fission is enabled, we should use
SpecialPowers.spawn instead of ContentTask.spawn because the iframes in the testcases
may be cross-origin iframes.

Comment 4

[Dimi Lee [:dimi]]

We still need the fix in Bug 1578465 to verify if we can enable these tests

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd12db83e342
Replace ContentTask.spawn with SpecialPowers.spawn for flash blocking testcases. r=gcp

Comment 6

[Natalia Csoregi [:nataliaCs]]

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

Comment 7

[Dimi Lee [:dimi]]

Attachment: Bug 1584931 - P1. Fix flashblock testcase failures when fission is enabled.

Comment 8

[Dimi Lee [:dimi]]

Hi kirk,
I have a patch which updates flashblock testcases to make them fission-compatible, but there is still one problem.
In some cases, child documents need their parent's flash classification result[1], which causes testcases here fail when fission is enabled because of we can't get parent's document across process boundary.
I am not super familiar with flashblock, do you know who might help to fix this? thank you!

Comment 9

[Robin Steuber (they/them) [:bytesized]]

I originally wrote the flash blocking code, but I'm not really sure what advice to offer. The specification for this feature requires that the child document check the origin and flash-block-status of parent documents.

I'm happy to answer additional questions about flash blocking, but I'm afraid that I'm not really knowledgeable enough about Fission to have much to suggest in terms of a fix.

Comment 10

[Neha Kochar [:neha]]

If flash classification result is not mutable, then it can be directly inherited from the parent like https://searchfox.org/mozilla-central/rev/8b7aa8af652f87d39349067a5bc9c0256bf6dedc/dom/ipc/BrowserBridgeParent.cpp#48-50. The other alternatives will be to add it to BrowsingContext or to WindowContext (under construction for now).
NI'ing Andreas for more help/pointers, if needed.

Comment 11

[Dimi Lee [:dimi]]

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

If flash classification result is not mutable, then it can be directly inherited from the parent like https://searchfox.org/mozilla-central/rev/8b7aa8af652f87d39349067a5bc9c0256bf6dedc/dom/ipc/BrowserBridgeParent.cpp#48-50. The other alternatives will be to add it to BrowsingContext or to WindowContext (under construction for now).

The approach adding it to BrowsingContext was what I planned to do in the first place, but then I realized the flash classification info we needed from the parent is a result calculated in the parent document[Here]. So It is not just a variable we can easily sync to BrowsingContext unless we listen to all the possible cases that may change the value and sync it. That's why I want to know if someone familiar with flashblock can help to see whether there is a better way to handle this.

Comment 12

[Andrew McCreight [:mccr8]]

If we're not planning on making plugins work with Fission, do we really need flashblock to work with Fission?

Comment 13

[Dimi Lee [:dimi]]

Attachment: Bug 1584931 - P1. Store the flash classification resut in BrowsingContext

This patch updates a flash classification result to BrowsingContext
whenever |DocumentFlashClassification| is called. The value is used
when a document requests a flash classification result from a
cross-parent document.

Note that we don't register any observer to preferences that may
change the outcome of Document::DocumentFlashClassification. I am not sure
if that is a case we need to worry about, but given that it may make
the code more complicated and plugins don't work in fission, the current
solution might just be good enough.

Comment 14

[Ethan Tseng [:ethan]]

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

If we're not planning on making plugins work with Fission, do we really need flashblock to work with Fission?

Hi Neha, could you or anyone confirm we don't support plugins with Fission?
If so, is there any documentation we could refer to for the record?

Comment 15

[Neha Kochar [:neha]]

After checking with cpeterson, its confirmed that we will need to support flash with fission.

Comment 16

[Andrew McCreight [:mccr8]]

Sorry for the confusion here! I'll hopefully review your patch today.

Comment 17

[Dimi Lee [:dimi]]

After spending more time figuring out the correct solution for this bug, I think we should probably store the flash classification result of a document to WindowContext instead of BrowsingContext because the result is per document.
Storing classification in BrowsingContext means that if a document is not active, we can't traverse the document chain correctly.

Comment 18

[Chris Peterson [:cpeterson]]

After discussing this bug with jimm (plugin module owner), we recommend just disabling the flashblock feature when Fission is enabled. The flashblock list is not actively maintained and Firefox's Flash support will be disabled in December 2020: https://developer.mozilla.org/en-US/docs/Plugins/Roadmap. So the period where Firefox users would have both Fission and Flash enabled is short or nil.

Comment 19

[Dimi Lee [:dimi]]

Attachment: Bug 1584931 - P2. Disable flashblock when fission is enabled.

This patch does the following:

1. Disable flashblock when fission is enabled.
2. Update flashblock tests to expect "unknown" classification when fission is
   enabled.
3. Remove skip-if=fission in flashblock mochitests.

Depends on D51098

Comment 20

[Dimi Lee [:dimi]]

Attachment: Bug 1584931 - P3. Hide the flashblock checkbox when fission is enabled.

Depends on D55091

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f7d7683109a
P1. Fix flashblock testcase failures when fission is enabled. r=bytesized,mccr8
https://hg.mozilla.org/integration/autoland/rev/e33d5fb3f370
P2. Disable flashblock when fission is enabled. r=bytesized
https://hg.mozilla.org/integration/autoland/rev/808799f11727
P3. Hide the flashblock checkbox when fission is enabled. r=Gijs

Comment 22

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/7f7d7683109a
https://hg.mozilla.org/mozilla-central/rev/e33d5fb3f370
https://hg.mozilla.org/mozilla-central/rev/808799f11727

Comment 23

[Cristian Fogel [:cfogel]]

Clearing qe+ since it's covered by automated tests.
Fwiw, did a quick check on 73.0a1(2019-12-05) Windows 10 - on y8 and encountered no crash.