Tony Chung [:tchung] Reporter
	Description • 12 years ago

I am unable to do an on-device screenshot for apps that are running Out Of Process.   take the screenshot, and when you open it, its just a blank white screen.

Apps affected:  Calendar, Browser, Calculator

Repro:
1) install otoro daily: 8-7-2012
2) launch an OOP app (Calendar, Browser, or Calculator)
3) hold down HOME and Power button, 2 vibrates.
4) screenshot notification appears
5) open the screenshot in a image viewer, and verify the image is showing blank white content.

Expected:
- screenshotting OOP apps should still show the content that it took

Actual:
- screenshot shows white blank screen

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 1 • 12 years ago

Are we doing a drawWindow() on the top-level chrome window with USE_WIDGET_LAYERS?  I tried to warn about this when we discussed screenshotting way back when ...

	David Flanagan [:djf]
	Comment 2 • 12 years ago

Yes, we're doing exactly that.  See b2g/chrome/content/shell.js:606

Note, though, that the screenshots are probably just a side-effect of something worse.  The more florid symptom is that in b2g-desktop (using yesterday's build) calculator and calendar display blank white screens.  Browser displays chrome, but the content area is always blank white.

Maybe this bug needs a different subject line?

	David Flanagan [:djf]
	Comment 3 • 12 years ago

I can reproduce the screenshot bug on otoro-gb running the most recent nightly.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 4 • 12 years ago

Seems plausible.  I don't see white screens on those apps with latest inbound on nexus s.  If we're failing to load apps OOP that would explain this bug.

	David Flanagan [:djf]
	Comment 5 • 12 years ago

CrystalSkull defaults to oop.  I've confirmed that screenshots are blank for it, and I get a blank white screen when I launch it on the desktop.

When I modified it to launch in process, I was able to take a screenshot on the otoro, and it displayed correctly on desktop.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 6 • 12 years ago

What happens when you screenshot it on device when running OOP?

	David Flanagan [:djf]
	Comment 7 • 12 years ago

Reread #5: screenshots work on the otoro when crystal skull is in process and they don't work when it is oop.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 8 • 12 years ago

I did, several times, which is why I asked explicitly :).

Interesting.  That's obviously not expected.  Same results with other OOP apps, like Tasks?

	David Flanagan [:djf]
	Comment 9 • 12 years ago

Every oop app I've tried has has the behavior, but I haven't tried them all.  I think tchung's intent with the subject line for this bug was to imply that it affected all oop apps.

	David Flanagan [:djf]
	Comment 10 • 12 years ago

On b2g desktop (on MacOS) all these OOP apps display plain blank screens.  But if you press and hold the Home button to bring up the task switcher, the screen is properly rendered in the screenshot view.

So I think this bug is related to https://bugzilla.mozilla.org/show_bug.cgi?id=781000

	Andrew Overholt [:overholt]
	Comment 11 • 12 years ago

basecamp-'ing based on the fact that it appears to only affect WebGL.  Re-nom if that's not correct :)

	David Flanagan [:djf]
	Comment 12 • 12 years ago

Renominating because https://bugzilla.mozilla.org/show_bug.cgi?id=781000 is blocking, and I suspect this is a related bug.

Also, see comment #2.  All of the OOP apps that don't screenshot on otoro don't display *at all* in b2g-desktop.  This is a serious WTF bug. We can't just say "webgl only"

Finally, if we don't ship a phone with working webgl, all the app writers who want to create games for our device are screwed.  Has a decision actually been made to not block for webgl?

	David Flanagan [:djf]
	Comment 13 • 12 years ago

I've filed a separate bug for the b2g-desktop piece of this: https://bugzilla.mozilla.org/show_bug.cgi?id=782411

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 14 • 12 years ago

*b2g-desktop-mac, unfortunately.

	Dave Hylands [:dhylands]
	Comment 15 • 12 years ago

I also observe the same behaviour on the otoro - snapshot of an OOP app shows up as all white, but works fine for non-OOP apps.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 16 • 12 years ago

We wouldn't hold the release for this, so not a blocker. But we should try to get this fixed sooner rather than later as to help QA.

	Jason Smith [:jsmith]
	Comment 17 • 12 years ago

This is more than just WebGL content - this is any content on the browser (just confirmed it's any content on the browser).

This is probably a blocker for dogfooding, doing compat analysis, etc. It's also impairing our ability to give "good" bugs - so I'm inclined to renom to reconsider this as a blocker for the dogfooding issue.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 18 • 12 years ago

We were aware of that during triage. It still didn't feel like we'd hold the release for this, which is the definition of a blocker (screenshots didn't seem like a required feature). But rather something we want to fix ASAP to help during development, which is why we gave it an assignment rather than blocker status.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 19 • 12 years ago

This blocks reftests.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 20 • 12 years ago

We're screenshotting from here

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ShadowLayersParent.cpp#484

which doesn't go through CompositorParent and doesn't apply async scroll offsets, take position:fixed layers into account, etc.  I thought that was fixed for fennec, but apparently not :|.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 21 • 12 years ago

Not to beat a dead horse, but rotation is another one of things not taken into account.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 23 • 12 years ago

Comment on attachment 663190 [details] [diff] [review]
Sketch of suggested fix for drawWindow()

Er yeah, not quite ready to land yet ;).

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 24 • 12 years ago

roc suggests moving this logic into the layer manager, which sounds good; everything above there will continue to Just Work.  We'll move the new logic here into

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/basic/BasicLayerManager.cpp#1093

which already sets up state in the way we need (mShadowTarget), but doesn't ask the right thing to render to the shadow target after the txn.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 25 • 12 years ago

Comment on attachment 663190 [details] [diff] [review]
Sketch of suggested fix for drawWindow()

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

Waiting for next patch

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 26 • 12 years ago

I have a working patch, but it turns out that LayerManagerOGL doesn't account for the world transform when reading back, so rotation is still broken.

	Tony Chung [:tchung] Reporter
	Comment 31 • 12 years ago

flagging one more time at Triage drivers for blocking-basecamp?, given the working patches that cjones has just submitted.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 32 • 12 years ago

Comment on attachment 663286 [details] [diff] [review]
part 2: Let clients query a widget's 'real' compositor, when the widget doesn't draw directly to its OS window

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

::: widget/xpwidgets/nsBaseWidget.cpp
@@ +925,5 @@
> +{
> +  if (!mCompositorChild) {
> +    return nullptr;
> +  }
> +  return nsRefPtr<CompositorChild>(mCompositorChild).forget();

Why not just return a raw CompositorChild*?

Also, rev nsIWidget IID

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 33 • 12 years ago

Comment on attachment 663287 [details] [diff] [review]
part 3: The ultimate compositor has to draw to the readback surface; shadow layers doesn't have access to all the needed rendering state

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

::: gfx/layers/basic/BasicLayerManager.cpp
@@ +1132,5 @@
> +          AutoOpenSurface opener(OPEN_READ_ONLY, snapshot);
> +          gfxASurface* source = opener.Get();
> +
> +          gfxContextAutoSaveRestore restore(mShadowTarget);
> +          mShadowTarget->SetOperator(gfxContext::OPERATOR_SOURCE);

I don't think we should use the SOURCE operator here. OVER is correct.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 34 • 12 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #32)
> Comment on attachment 663286 [details] [diff] [review]
> part 2: Let clients query a widget's 'real' compositor, when the widget
> doesn't draw directly to its OS window
> 
> Review of attachment 663286 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/xpwidgets/nsBaseWidget.cpp
> @@ +925,5 @@
> > +{
> > +  if (!mCompositorChild) {
> > +    return nullptr;
> > +  }
> > +  return nsRefPtr<CompositorChild>(mCompositorChild).forget();
> 
> Why not just return a raw CompositorChild*?
> 

I can do that.

> Also, rev nsIWidget IID

Oh right.  I keep dreaming that I finished that de-COM patch ...

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 35 • 12 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #33)
> Comment on attachment 663287 [details] [diff] [review]
> part 3: The ultimate compositor has to draw to the readback surface; shadow
> layers doesn't have access to all the needed rendering state
> 
> Review of attachment 663287 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/basic/BasicLayerManager.cpp
> @@ +1132,5 @@
> > +          AutoOpenSurface opener(OPEN_READ_ONLY, snapshot);
> > +          gfxASurface* source = opener.Get();
> > +
> > +          gfxContextAutoSaveRestore restore(mShadowTarget);
> > +          mShadowTarget->SetOperator(gfxContext::OPERATOR_SOURCE);
> 
> I don't think we should use the SOURCE operator here. OVER is correct.

OK, fixed.

	Jason Smith [:jsmith]
	Comment 37 • 12 years ago

Clearing the basecamp nom - its gone through triage a few times and has been determined to not block ship, mainly since having screenshots broken won't stop ship.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 38 • 12 years ago

I don't really care if this is blocking+ or -; it prevents us from running meaningful reftests.  As I recall, at some points in the past, that's been marked blocking+, and at other times blocking-.  Whatever.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 39 • 12 years ago

There are some disturbing failures on tryserver with segfaults accessing the size tag of shmem segments (AFAICT) that should already be allocated, mapped, and committed.  Yay.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 40 • 12 years ago

Repushed to see if the android fairies have fixed things in the meantime

https://tbpl.mozilla.org/?tree=Try&rev=455ea74b1a57

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 41 • 12 years ago

The gods were not kind to me.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 42 • 12 years ago

After the fixes to reftest-remote, the harness still wouldn't launch, because this function was throwing and aborting the tests.  No idea what's going on here.  This patch could get me in trouble with Johnson & Johnson, so feel free to f-.

After spending 30 minutes trying to get various flavors of --disable-optimize builds to terminate linking, I'm taking a break to go land some other code :|.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 43 • 12 years ago

(good news, though, is that I can still repro what looks like the same crash locally.)

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 45 • 12 years ago

Gave up on trying to get any more information out of android.  I reproduced the crash x11/omtc though.  A debug build would have told me what the problem was in about 5 seconds :(.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 46 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/53bf054fe0f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/abefcbf2f2ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/c18ffc64e3d0

	Ed Morley [:emorley]
	Comment 47 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/53bf054fe0f1
https://hg.mozilla.org/mozilla-central/rev/abefcbf2f2ee
https://hg.mozilla.org/mozilla-central/rev/c18ffc64e3d0

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 48 • 12 years ago

Chris - Do we still need the bandaid fix?

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 49 • 12 years ago

I wasn't able to run reftest-remote locally without it.