Closed Bug 1592895 Opened 5 years ago Closed 4 years ago

Audit usage of Document::EnumerateSubDocuments in layout for Fission

Categories

(Core :: Layout, task, P3)

task

Tracking

()

RESOLVED FIXED
Fission Milestone M6a

People

(Reporter: hiro, Assigned: hiro)

References

(Blocks 1 open bug)

Details

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.

Depends on: 1596317

Tracking for Fission Nightly (M6)

Fission Milestone: --- → M6
Summary: Audit usage of Document::EnumerateSubDocuments in layout → Audit usage of Document::EnumerateSubDocuments in layout for Fission
Depends on: 1640768

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.

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.

Severity: normal → N/A
Flags: needinfo?(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.

Fission Milestone: M6 → M6a
Flags: needinfo?(emilio)

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.
  • 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?
Flags: needinfo?(tnikkel)
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(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.

Flags: needinfo?(emilio)

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

Whiteboard: [7/15] waiting for matt.woodrow's and tnikkel's inputs, then we can close as the auditing work will be complete

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

Flags: needinfo?(matt.woodrow)

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

Flags: needinfo?(tnikkel)

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

Assignee: nobody → hikezoe.birchill
Status: NEW → RESOLVED
Closed: 4 years ago
Depends on: 1649065
Resolution: --- → FIXED
Whiteboard: [7/15] waiting for matt.woodrow's and tnikkel's inputs, then we can close as the auditing work will be complete
Blocks: 1656105
You need to log in before you can comment on or make changes to this bug.