Open Bug 1584931 Opened 2 months ago Updated 9 hours ago

Fix flashblock test failures when fission is enabled

Categories

(Toolkit :: Safe Browsing, defect, P1)

defect

Tracking

()

ASSIGNED
Fission Milestone M4.1

People

(Reporter: dimi, Assigned: dimi, NeedInfo)

References

(Blocks 1 open bug)

Details

(Keywords: leave-open)

Attachments

(3 files)

No description provided.
Depends on: fission-mochitests

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!

Flags: needinfo?(kmaglione+bmo)
Depends on: 1586790

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.

Depends on: 1578465
No longer depends on: fission-mochitests, 1586790
Flags: needinfo?(kmaglione+bmo)
Fission Milestone: --- → M4

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.

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

Keywords: leave-open
Pushed by dlee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bd12db83e342
Replace ContentTask.spawn with SpecialPowers.spawn for flash blocking testcases. r=gcp

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!

Flags: needinfo?(ksteuber)

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.

Flags: needinfo?(ksteuber)

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.

Flags: needinfo?(afarre)

(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.

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

Attachment #9105224 - Attachment is obsolete: true

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.

Attachment #9105224 - Attachment description: Bug 1584931 - P1. Fix flashblock testcase failures when fission is enabled. → Bug 1584931 - P2. Fix flashblock testcase failures when fission is enabled.
Attachment #9105224 - Attachment is obsolete: false

(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?

Flags: needinfo?(nkochar)
Fission Milestone: M4 → M4.1
You need to log in before you can comment on or make changes to this bug.