1592895 - Audit usage of Document::EnumerateSubDocuments in layout for Fission

Status: RESOLVED FIXED

(Core :: Layout, task, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

I think it doesn't work in fission world, Emilio's comment (bug 1503656 comment 23) makes me think it, I haven't checked the behavior though.

Comment 1

[Chris Peterson [:cpeterson]]

Tracking for Fission Nightly (M6)

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Note for EnumerateSubDocuments call in DocHasPrintCallbackCanvas;

IIUC, mozPrintCallback is only used when pdf viewer tries to print and I suppose there is no OOP iframes in PDF documents, so we don't need anything for the EnumerateSubDocuments call.

Comment 3

[Sean Voisen (:svoisen)]

Emilio: Can you help set the milestone to M6a/b/c? I'm unsure how important this audit work is to enabling in Nightly, but feels fairly important.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Most of them seem broken on fission:

• Freezing pres-shells should probably freeze OOP iframes too. That seems broken.
• FlushThrottledStyles and such are probably not a big deal. If we target the OOP iframe we'll run it there.
• Should probably dispatch beforeprint and afterprint to OOP iframes too (that's this one). I don't know what our story for printing fission iframes is though, but that seems broken.
• For resolution changes it's not clear if the existing support to propagate this to remote browsers is enough (this is it). Test needed if so, fix needed otherwise?
• For media feature changes, the callers seem to deal with propagating to all processes so may be ok, if a bit sketchy.
• PresShell::EndPaint / NotifyDidPaintForSubtree may be ok, subdocuments have their own painting story.
• ReduceAnimations is probably fine as well (OOP subframes will have their own refresh driver).
• Unclear if the SwapDocShells machinery needs to do something about OOP iframes. This is the "move tab to another window" machinery. If it works with OOP iframes already we're probably fine.
• RetainedDisplayListBuilder and the other animation callers are probably alright, OOP iframes have their own animation timelines etc.
• mozPrintCallback is broken but as hiro mentions in comment 2 it probably doesn't matter much.

I think we should double-check the list above soonish, and file bugs for the broken ones sooner rather than later.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

I can answer/audit some of them and can defer appropriate person. :)

• FlushThrottledStyles and such are probably not a big deal. If we target the OOP iframe we'll run it there.
  • I've confirmed that FlushThrottledStyles is processed in an OOP iframe.
• Should probably dispatch beforeprint and afterprint to OOP iframes too (that's this one). I don't know what our story for printing fission iframes is though, but that seems broken.
  • This has already been filed, bug 1640768.
• For media feature changes, the callers seem to deal with propagating to all processes so may be ok, if a bit sketchy.
  • Yep, I think so. I've already closed bug 1604078 which is for the media feature issue.
• ReduceAnimations is probably fine as well (OOP subframes will have their own refresh driver).
  • Right, this is not a problem.
• mozPrintCallback is broken but as hiro mentions in comment 2 it probably doesn't matter much.
  • yep, this is not a problem.
• For resolution changes it's not clear if the existing support to propagate this to remote browsers is enough (this is it). Test needed if so, fix needed otherwise?
  • Tested locally, confirmed it works.
• The other animation callers are probably alright, OOP iframes have their own animation timelines etc.
  • Yep, that's right.

So rest of them are;

• Freezing pres-shells should probably freeze OOP iframes too. That seems broken.
  • I believe Emilio is a right person to ask. Emilio, have you ever filed a bug for this?
• PresShell::EndPaint / NotifyDidPaintForSubtree may be ok, subdocuments have their own painting story.
  • Matt, can you audit this?
• Unclear if the SwapDocShells machinery needs to do something about OOP iframes. This is the "move tab to another window" machinery. If it works with OOP iframes already we're probably fine.
  • Timothy?
• RetainedDisplayListBuilder.
  • Also Matt?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Freezing pres-shells should probably freeze OOP iframes too. That seems broken.

I haven't but it looks like freezing is only in order to put the pres shell on the BFCache. I think we only store the presentation of the top level document, but icbw. In any case, this should probably be figured out as part of the BFCache changes for Fission.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)

Freezing pres-shells should probably freeze OOP iframes too. That seems broken.

I haven't but it looks like freezing is only in order to put the pres shell on the BFCache. I think we only store the presentation of the top level document, but icbw. In any case, this should probably be figured out as part of the BFCache changes for Fission.

Thank you, Emilio, I filed bug 1649065 for that (in DOM:Navigation).

Comment 8

[Matt Woodrow (:mattwoodrow)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)

• PresShell::EndPaint / NotifyDidPaintForSubtree may be ok, subdocuments have their own painting story.
  • Matt, can you audit this?
• RetainedDisplayListBuilder.
  • Also Matt?

All of these should be fine to only recurse the in-process subdocuments.

Comment 9

[Timothy Nikkel (:tnikkel)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)

• Unclear if the SwapDocShells machinery needs to do something about OOP iframes. This is the "move tab to another window" machinery. If it works with OOP iframes already we're probably fine.
  • Timothy?

I agree. Anything broken here would also be broken for the case of a top-level out of browser element, which is a basic thing we do all the time, so we would be aware if it was broken. So this shouldn't need any change for fission.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Thank you all involved here in this bug!

I am going to close this bug (assigning to myself just in case if we need an owner).