574529 - Allow ourselves to flush layout in Document A while Document B is painting

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

For SVG-as-image (bug 276431) to work without massive hackery, we need to be able to dynamically set the helper SVG-document's viewport size (via  nsIContentViewer::SetBounds) **and flush any resulting layout changes** within imgIContainer::Draw, at paint-time.

(We may be painting multiple instances of the same SVG image with radically different dimensions, or we may just be painting one instance and we've only just now figured out what size we want it to be.)

Currently this doesn't work, because we're painting, and so PresShell::IsSafeToFlush() returns PR_FALSE, and so our attempted layout-flush gets ignored.  However, it should be fine to honor our layout-flush, because we're painting a *different document* from the one whose layout we'd be flushing.  So, while this may cause a performance hit in pathological cases, I think it should be fine to allow ourselves to flush layout in this situation.

The main case where it would be a performance hit would be with HTML documents that have many instances of the same SVG image at different viewport-sizes (making us switch the helper-document's bounds back and forth each time we paint the full page).  That's expected to be an uncommon case.  Generally, I'd expect that an SVG image would only be used at a single size in a particular document.

So, I'm filing this bug on making PresShell::IsSafeToFlush() only care about whether **our document** (PresShell::mDocument) is being currently painted, so we can flush the effects of SetBounds in the helper-doc while the host document is being painted.

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to comment #0)
> However, it should be fine to honor our layout-flush, because
> we're painting a *different document* from the one whose layout we'd be
> flushing.

(Note that we've received a request -- imgIContainer::Draw -- to paint the document whose layout we'd be flushing, the helper-SVG-document.  But we haven't actually started painting it yet.)

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Probably we should check whether the RootPresContext() for the document we want to flush in is the RootPresContext() for something that's currently being painted.

Comment 3

[Daniel Holbert [:dholbert]]

One other thing that we need to take care of to make this work:
FlushPendingNotifications() also checks "nsContentUtils::IsSafeToRunScript()" before actually flushing, and that trips us up too, because nsViewManager::Refresh (the top-level painting method) creates a script-blocker.

In the case of our SVG helper-document, though, scripts are disabled.  So, perhaps FlushPendingNotifications can ignore "IsSafeToRunScript" if scripting is disabled?  (as reported by mDocument->ScriptLoader()->GetEnabled())

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Probably we should check whether the RootPresContext() for the document we
> want to flush in is the RootPresContext() for something that's currently being
> painted.

Don't we already?  More precisely, we check that the root view manager for the document we plan to flush is being painted.  So I'm surprised we're hitting this; I would think the external SVG document is not hooked up into the main-page viewmanager hierarchy....

The approach described in comment 0 doesn't work, by the way, because in general a flush can run arbitrary script, which can cause it to reach outside the document it's in and tear down the world.  Including whatever is painting.

Now in this case we happen to know that it won't, because there's no scripting allowed in the document in question.  So we could check for that.  But I'm still not sure why this is a problem at all.

Comment 5

[Daniel Holbert [:dholbert]]

Hmm, given comment 4, it might actually only have been the script-blocker that's blocking the layout-flush (per comment 3), and not the IsPainting() stuff after all.  Checking, one sec.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #3)
> In the case of our SVG helper-document, though, scripts are disabled.  So,
> perhaps FlushPendingNotifications can ignore "IsSafeToRunScript" if scripting
> is disabled?  (as reported by mDocument->ScriptLoader()->GetEnabled())

That sounds good to me!

Comment 7

[Boris Zbarsky [:bzbarsky]]

Note that the check in comment 6 might not be good enough, in general.  Just because loading of new scripts is disabled doesn't mean existing compiled scripts are disabled, sadly, nor does it prevent compilation+execution of event handlers.  I'd prefer a stricter check: check that mDocument has no script global.

Comment 8

[Daniel Holbert [:dholbert]]

Yup, comment 5 is correct -- it's just the script-blocker that's blocking us after all.  (sorry about that -- been a little while since I tried to actually do the flush while painting -- because it was getting blocked -- and I forgot which component(s) of isSafeToFlush boolean in FlushPendingNotifications had been responsible.  Turns out it's just the script blocker.)

/me hacks up a patch along the lines of comment 7.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: fix

This patch only has us care about nsContentUtils::IsSafeToRunScript() if GetScriptGlobalObject() returns something non-null, as suggested in comment 7.  This seems to work for me.

Comment 10

[Daniel Holbert [:dholbert]]

Requesting blocking, since this is needed for beta5-blocker-bug 276431.

Comment 11

[Daniel Holbert [:dholbert]]

Per bug 276431 comment 169, this doesn't need to block beta5 - it can now block beta6 instead. (since this bug is a helper for bug 276431, which is moving to beta6)

Comment 12

[Daniel Holbert [:dholbert]]

http://hg.mozilla.org/mozilla-central/rev/574cf874df05