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

RESOLVED FIXED in mozilla2.0b7

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla2.0b7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

Attachments

(1 attachment)

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.
(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.)
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.
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())
> 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.
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.
(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!
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.
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.
Posted patch fixSplinter Review
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.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #453947 - Flags: review?(bzbarsky)
Attachment #453947 - Flags: review?(bzbarsky) → review+
Blocks: 276431
Requesting blocking, since this is needed for beta5-blocker-bug 276431.
blocking2.0: --- → ?
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)
blocking2.0: beta5+ → beta6+
http://hg.mozilla.org/mozilla-central/rev/574cf874df05
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6
Blocks: 634291
You need to log in before you can comment on or make changes to this bug.