Closed Bug 1584931 Opened 1 year ago Closed 1 year ago

Disable flashblock list and tests when Fission is enabled

Categories

(Toolkit :: Safe Browsing, task, P1)

task

Tracking

()

RESOLVED FIXED
mozilla73
Fission Milestone M4.1
Tracking Status
firefox72 --- wontfix
firefox73 --- fixed

People

(Reporter: dimi, Assigned: dimi)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

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: WPT-Fis

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, WPT-Fis
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

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

Flags: needinfo?(nkochar)

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

Flags: needinfo?(afarre)

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.

Depends on: WindowContext

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.

Type: defect → task
Summary: Fix flashblock test failures when fission is enabled → Disable flashblock list and tests when Fission is enabled
Attachment #9107842 - Attachment is obsolete: true
Attachment #9105224 - Attachment is obsolete: true
Attachment #9105224 - Attachment description: Bug 1584931 - P2. Fix flashblock testcase failures when fission is enabled. → Bug 1584931 - P1. Fix flashblock testcase failures when fission is enabled.
Attachment #9105224 - Attachment is obsolete: false

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

Attachment #9112255 - Attachment description: Bug 1584391 - P2. Disable flashblock when fission is enabled. → Bug 1584931 - P2. Disable flashblock when fission is enabled.
Attachment #9112256 - Attachment description: Bug 1584391 - P3. Hide the flashblock checkbox when fission is enabled. → Bug 1584931 - P3. Hide the flashblock checkbox when fission is enabled.
Pushed by dlee@mozilla.com:
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
Keywords: leave-open
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla73
Flags: qe-verify+
See Also: → 1609791

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.

Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.