chore: disable certain Fission features for Firefox #7610
Conversation
|
With all the checks passing, I'm close to request review for this change. But there is one question remaining. @jschfflr does the order of emitted events actually matter in Puppeteer or CDP? It doesn't look like. Background here is that with enabling Fission in Firefox we have a lot more async code, and as such event order is mixed up compared to what we emit without Fission or what Chrome emits. As example, when navigating away from a frameset page, the contained frame is destroyed after the execution context of the new page has been created. Can we assume that the order doesn't matter? If that is the case I would remove strict event order assertions in our own internal tests for CDP. With that done we could enable Fission with some features still disabled, which is a great step forward. Related bug here is https://bugzilla.mozilla.org/show_bug.cgi?id=1601245. |
whimboo
force-pushed
the
firefox_fission_support
branch
from
d192971
to
4a6fb8e
Compare
|
cc @OrKoN as well |
|
From our side the landing of this PR actually blocks any work for issue #6894. |
|
@whimboo For the OOP iframe support that I'm currently working on, there is the implicit assumption of a frame being attached to its parent frame again before the target is destroyed if the frame is moving from OOP to in process again. But I don't think we have them documented anywhere. But I'm pretty sure there will be a bunch of them. |
|
Thanks @jschfflr! Do you know if this OOP support in Puppeteer is something that will ship in Q4 this year? If yes is there something that I have to be aware of? Happy to chat about that via the #webdriver Element/Matrix channel. Thanks in advance. |
|
FYI I'll keep this PR as draft for now until we have merged these changes in our own repository first. This might take a couple days. |
|
Thanks! So given that current Puppeteer tests are passing, does Puppeteer also pass with Chrome having OOP enabled? Or do you force it off for the time being? If you do, we may want to retain the |
|
@jschfflr we are kinda interested to get this change into the next Puppeteer release. Is there already a time scheduled for it, or do we have some more days to get our CI polished up. Nevertheless we might be ok in shipping these changes. Thanks. |
|
As heads-up, my patches on https://bugzilla.mozilla.org/show_bug.cgi?id=1601245 have been landed. If all goes well these will reach the Firefox Nightly release today. Our internal CI hasn't shown any failures for Puppeteer whether if Fission is enabled or disabled (as per default right now). @jschfflr if you notice failures today please let me know. If it works well I will ask for uplifting the patch to Firefox 94 beta. Once it reaches release we can continue to work on issue #6894. Having no failures we could consider merging this PR tomorrow or on Friday. |
|
Internal tests are working great with this change. So I do not see a reason why we would have to delay the merge of this PR anytime longer. @jschfflr could you please merge? Thanks! |
|
@whimboo Awesome, thanks! Happy to get this landed :) |
|
@jschfflr thanks for merging. Do you already know when the next Puppeteer release is due? In about a months time work will be started to disallow turning off Fission in Firefox. As such this will negatively affect Puppeteer users. So getting a release shipped until then would be great. Thanks. |
Enable basic Fission support for Firefox' CDP implementation. Some other Fission specific features have to be kept disabled for now.