369146 - nsCanvasRenderingContext2D::DrawWindow should flush layout on child frames

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

A possible cause for the reftest failure in bug 1156 comment 78 is that nsCanvasRenderingContext2D::DrawWindow does not flush layout on subframes.  I haven't been able to get the failure to happen again, though, so I can't really be sure.

Comment 1

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: possible patch

I'm not sure how much error handling and null-checking I need to do when using the doc shell API.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 253802 [details] [diff] [review]
possible patch

>diff -r 77cf62b10152 content/canvas/src/nsCanvasRenderingContext2D.cpp
>+    if (domDoc) {
>+        nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc);

Don't really need to null-check |domDoc|, since do_QI is null-safe.

>+    nsCOMPtr<nsIDocShellTreeNode> node =
>+        do_QueryInterface(piWin->GetDocShell());

Do need to null-check |node| here, though.  If the flush tore down an ancestor nsSubDocumentFrame and it was a <xul:iframe> or <xul:browser>, the window won't have a docshell.

>+        if (shell) {
>+            nsCOMPtr<nsIDOMWindow> win = do_GetInterface(shell);

do_GetInterface is also null-safe.

And the other null-checks I think you do need.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I should note that because FlushPendingNotifications flushes parents this is sorta O(N^2) in docshell tree depth.  But the tree never gets all that deep, hopefully....  If we care about that, we should only flush on docshells which have no kids we flushed on.

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: patch addressing comments

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Checked in to trunk.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Testcase

If you happen to have a tree from before this change, does this testcase fail?

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 253831 [details] [diff] [review]
Testcase

I don't have a tree from before the change; probably the easiest way to get one is to comment out the bottom part of FlushLayoutToTree that makes the recursive calls.  It would be good to double-check that the testcase fails pre-patch (could you comment on the bug if you do?), but probably good to get it checked in either way.