1310745 - Make asynchronous plugin painting pretend that the prescontext has a pending paint

Status: RESOLVED WONTFIX

(Core :: Graphics, defect, P3)

Description

[(no longer active)]

From bug 1279086 comment 49:

> Is this stack possible, and not a duplicate?
> 
> void PresShell::Paint(nsView*, nsRegion*, uint32)
> uint32 nsLayoutUtils::PaintFrame(nsRenderingContext*, nsIFrame*, nsRegion*,
> uint32, uint8, uint32)
> void nsIFrame::BuildDisplayListForStackingContext(nsDisplayListBuilder*,
> nsRect*, nsDisplayList*)
> void nsPluginFrame::BuildDisplayList(nsDisplayListBuilder*, nsRect*,
> nsDisplayListSet*)
> void nsPluginInstanceOwner::NotifyPaintWaiter(nsDisplayListBuilder*)
> void nsContentUtils::AddScriptRunner(nsIRunnable*)
> void nsContentUtils::AddScriptRunner(struct already_AddRefed<nsIRunnable>)
> uint32 nsAsyncScriptLoad::Run()
> void nsInProcessTabChildGlobal::LoadFrameScript(nsAString_internal*, uint8)
> void nsMessageManagerScriptExecutor::LoadScriptInternal(nsAString_internal*,
> uint8)
> uint8 js::ExecuteInGlobalAndReturnScope(JSContext*, class
> JS::Handle<JSObject*>, class JS::Handle<JSScript*>, class
> JS::MutableHandle<JSObject*>)
> uint8 js::ExecuteKernel(JSContext*, class JS::Handle<JSScript*>, JSObject*,
> JS::Value*, js::AbstractFramePtr, JS::Value*)
> uint8 js::RunScript(JSContext*, js::RunState*)
> Interpreter.cpp:uint8 Interpret(JSContext*, js::RunState*)

This is sadly possible.  Perhaps we need to always hold a script blocker during painting, thanks to NotifyPaintWaiter().




I think a good way of alleviating this and other similar potential cases is adding a script blocker to PresShell::Paint().  I can't think of what this could break...  Needinfoing Boris and Olli in case they can think of something?

Comment 1

[Boris Zbarsky [:bzbarsky]]

At first glance that sounds sane.  But I would be sadly unsurprised if it broke something too.  :(

Comment 2

[(no longer active)]

I guess I'll post something to try for now...

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'd like to understand better what nsPluginInstanceOwner::NotifyPaintWaiter is trying to do. The event it fires (MozPaintWait) is intended to be used only by reftests. If we delayed firing that event, would we break reftests? Either way, we probably shouldn't be firing it on normal web pages (which, as far as I can tell, we are doing).

Also, a script blocker in PresShell::Paint won't help with bug 1279086. The scripts would still run when the paint finishes. We instead need to delay them until the content JS that's on the stack finishes. We could add a different kind of script blocker for that, but I'm not sure it's needed.

Comment 4

[(no longer active)]

(In reply to Bill McCloskey (:billm) from comment #3)
> I'd like to understand better what nsPluginInstanceOwner::NotifyPaintWaiter
> is trying to do. The event it fires (MozPaintWait) is intended to be used
> only by reftests. If we delayed firing that event, would we break reftests?
> Either way, we probably shouldn't be firing it on normal web pages (which,
> as far as I can tell, we are doing).

I searched through the code history a bit.  It seems that in bug 611164 roc modified this code in order to fix an intermittent reftest failure.  I sort of gave up looking further back in time.

Matt, do you happen to know what this is used for?

> Also, a script blocker in PresShell::Paint won't help with bug 1279086. The
> scripts would still run when the paint finishes. We instead need to delay
> them until the content JS that's on the stack finishes. We could add a
> different kind of script blocker for that, but I'm not sure it's needed.

Ah, right.  Adding a different type of script blocker is probably fine, but I also prefer to not do that if we can avoid it...

Comment 5

[(no longer active)]

I did a bit more archaeology on this.  The code that adds the script runner was added in http://hg.mozilla.org/mozilla-central/rev/955ba94047fd.  In that patch, there's a code path that dispatches the event asynchronously off the main thread, and there is another code path which seems to try to dispatch the event at the end of PresShell::RenderDocument().

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

I have nothing to add to comment 1, 
except that could we get rid of nsPluginInstanceOwner::NotifyPaintWaiter or make it clearly a testing only thing? reftests aren't ever run in a setup where there are multiple tabs, I think.

Comment 7

[Matt Woodrow (:mattwoodrow)]

I don't really know that code, but I think I can see why it's there.

Once the reftest harness has received notification of an update (MozAfterPaint) and takes an updated snapshot, it then checks to see if any further paints are pending (nsIDOMWindowUtils::IsMozAfterPaintPending, nsPresContext::IsDOMPaintEventPending).

If there are no pending paints, then we've reached a stable state and the test is considered complete.

Async plugins can have work remaining even though the pres context doesn't have any scheduled paints, so we would end up ending the test without the plugin content being available.

It looks like MozPaintWait is an explicit notification to the reftest harness that it needs to wait longer, and should be matched MozPaintWaitFinished to notify the harness that it may finish the test (modulo the normal end conditions).

I think we could make nsPresContext::IsDOMPaintEventPending take any async plugins into account (possibly by have NotifyPaintWaiter find the right pres context and tell it to wait), then we wouldn't need the explicit event.

We would need to make sure that a MozAfterPaint always gets sent if we ever returned true from IsDOMPaintEventPending, but we can fire the event with an empty region if necessary (we do this already).

Comment 8

[(no longer active)]

Attachment: Make asynchronous plugin painting pretend that the prescontext has a pending paint

Comment 9

[(no longer active)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a1738be5868d1fab3026aafd163db10ccf58b49

Comment 10

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8802179 [details] [diff] [review]
Make asynchronous plugin painting pretend that the prescontext has a pending paint

Review of attachment 8802179 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/base/nsPluginInstanceOwner.cpp
@@ +149,5 @@
> +    if (!presShell) {
> +      return;
> +    }
> +
> +    nsPresContext *presContext = presShell->GetPresContext();

I think we need to find the root prescontext and set the flag there.

If the plugin is in a subdocument, then we still need to make sure IsDOMPaintPending returns true for the root document (which is what reftests will check via nsIDOMWindowUtils).

@@ +595,5 @@
> +        nsIPresShell *presShell = doc->GetShell();
> +        if (presShell) {
> +          nsPresContext *presContext = presShell->GetPresContext();
> +          if (presContext) {
> +            presContext->NotifyPluginPaintFinished();

Can we assert (probably NS_ASSERTION?) if any of these checks failed?

If we've called NotifyWaitingForPluginPaint (mWaitingForPaint is true), but we can't call NotifyPluginPaintFinished then isMozAfterPaintPending will be true forever and the reftest harness will hang.

There are other users of isMozAfterPaintPending too, including addons iirc.

::: layout/base/nsPresContext.h
@@ +1184,5 @@
>  #endif
>  
>    void InvalidatePaintedLayers();
>  
> +  void NotifyWaitingForPluginPaint() { mWaitingForPluginPaint = true; }

Isn't it possible to have multiple plugins within a document that might call this? Shouldn't we have a count of plugins to wait for, rather than just a boolean.

Comment 11

[(no longer active)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> Comment on attachment 8802179 [details] [diff] [review]
> Make asynchronous plugin painting pretend that the prescontext has a pending
> paint
> 
> Review of attachment 8802179 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/plugins/base/nsPluginInstanceOwner.cpp
> @@ +149,5 @@
> > +    if (!presShell) {
> > +      return;
> > +    }
> > +
> > +    nsPresContext *presContext = presShell->GetPresContext();
> 
> I think we need to find the root prescontext and set the flag there.
> 
> If the plugin is in a subdocument, then we still need to make sure
> IsDOMPaintPending returns true for the root document (which is what reftests
> will check via nsIDOMWindowUtils).

Sure, makes sense!

> @@ +595,5 @@
> > +        nsIPresShell *presShell = doc->GetShell();
> > +        if (presShell) {
> > +          nsPresContext *presContext = presShell->GetPresContext();
> > +          if (presContext) {
> > +            presContext->NotifyPluginPaintFinished();
> 
> Can we assert (probably NS_ASSERTION?) if any of these checks failed?
> 
> If we've called NotifyWaitingForPluginPaint (mWaitingForPaint is true), but
> we can't call NotifyPluginPaintFinished then isMozAfterPaintPending will be
> true forever and the reftest harness will hang.

Sure.

> There are other users of isMozAfterPaintPending too, including addons iirc.

Yeah, the in-tree ones basically all expect a MozAfterPaint event to be dispatched in the future if this attribute is true.  There doesn't seem to be any important uses in add-ons: <https://dxr.mozilla.org/addons/search?q=isMozAfterPaintPending&redirect=false>

> ::: layout/base/nsPresContext.h
> @@ +1184,5 @@
> >  #endif
> >  
> >    void InvalidatePaintedLayers();
> >  
> > +  void NotifyWaitingForPluginPaint() { mWaitingForPluginPaint = true; }
> 
> Isn't it possible to have multiple plugins within a document that might call
> this? Shouldn't we have a count of plugins to wait for, rather than just a
> boolean.

Yes, you're right!

Comment 12

[(no longer active)]

Comment on attachment 8802179 [details] [diff] [review]
Make asynchronous plugin painting pretend that the prescontext has a pending paint

Review of attachment 8802179 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/tools/reftest/reftest-content.js
@@ +666,3 @@
>          var painted = SendInitCanvasWithSnapshot();
>  
> +        if (shouldWaitForPendingPaints() ||

I also found out that this change was wrong, since now the plugin async paints go through the normal path, so doing a check for a pending paint here, before the canvas snapshot initiated above has had a chance to finish, could make us get into a loop of starting to wait for a paint before we've received the MozAfterPaint event we were originally anticipating, which showed up as reftest timeouts on try (they were really the reftest framework painting in an infinite loop.)

Comment 13

[(no longer active)]

Attachment: Make asynchronous plugin painting pretend that the prescontext has a pending paint

Comment 14

[(no longer active)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dce945c234a8182080d0ff89dc2305a48128ca99

Comment 15

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8802285 [details] [diff] [review]
Make asynchronous plugin painting pretend that the prescontext has a pending paint

Review of attachment 8802285 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/crashtests/crashtests.list
@@ -152,5 @@
>  load 374297-2.html
>  load 376223-1.xhtml
>  load 378325-1.html
>  load 378682.html
> -load 379105-1.xhtml

Why this change?

::: layout/tools/reftest/reftest-content.js
@@ +462,5 @@
>          switch (state) {
>          case STATE_WAITING_TO_FIRE_INVALIDATE_EVENT: {
>              LogInfo("MakeProgress: STATE_WAITING_TO_FIRE_INVALIDATE_EVENT");
> +            if (shouldWaitForPendingPaints() &&
> +                shouldWaitForPendingPaints()) {

We don't need this twice, do we?

Comment 16

[(no longer active)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #15)
> Comment on attachment 8802285 [details] [diff] [review]
> Make asynchronous plugin painting pretend that the prescontext has a pending
> paint
> 
> Review of attachment 8802285 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/crashtests/crashtests.list
> @@ -152,5 @@
> >  load 374297-2.html
> >  load 376223-1.xhtml
> >  load 378325-1.html
> >  load 378682.html
> > -load 379105-1.xhtml
> 
> Why this change?
> 
> ::: layout/tools/reftest/reftest-content.js
> @@ +462,5 @@
> >          switch (state) {
> >          case STATE_WAITING_TO_FIRE_INVALIDATE_EVENT: {
> >              LogInfo("MakeProgress: STATE_WAITING_TO_FIRE_INVALIDATE_EVENT");
> > +            if (shouldWaitForPendingPaints() &&
> > +                shouldWaitForPendingPaints()) {
> 
> We don't need this twice, do we?

Both are accidental changes, which I should have removed.

I tried to reproduce the failures on try on Windows, and couldn't.  So it's unlikely I can make a lot of progress on this before I leave on vacation.  I'd appreciate if someone else can pick this up.

Comment 17

[(no longer active)]

Bill, can you see if you can find another owner for this please?

The current status is that my patches break the reftests in dom/plugins on Windows, but I was not able to reproduce the failures locally.

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Now that bug 1308039 has been backed out, this is less of a priority. It's still something we should do, of course.

Comment 19

[Gregory Pappas [:gregp]]

Plugins are gone