Closed Bug 1178823 Opened 9 years ago Closed 9 years ago

Black screen on a machine that the sanity test failed to detect

Categories

(Core :: Graphics, defect)

41 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: mchang, Assigned: mchang)

References

Details

Attachments

(3 files, 2 obsolete files)

From the daily today, it looks like we have one machine in the Toronto office that loads Firefox with a black screen and according to telemetry, still passed the gfx sanity test.

Layers was already blacklisted and the machine defaulted back to WARP, but still showed a black screen. Disabling layers acceleration and hence defaulting to the basic compositor fixed the issue.

Find out what's going on with the sanity test, why we failed to detect the issue, and improve the sanity test to find this case.
Bug 1178368 is about fixing the actual problem.
Try build with lots of logging:

https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/mchang@mozilla.com-cf0cf314da28

Should be done in an hour or two.
Blocks: 1178447
Can you please try the builds from comment 2 and output the debug log? The sanity test window should also stay open, so can you please get a screenshot of that as well? Thanks!
Flags: needinfo?(matt.woodrow)
I was able to see a black screen with Windows 7 with remote desktop on yesterdays' nightlies. 41.0a1, 2015-06-29. When Nightly updated itself to today, 2015-06-30, the problem went away. Can you try 2015-06-29 nightly and see if the black screen reproduces?
These look interesting:

22bc3c641838	Matt Woodrow — Bug 1176570 - Make sure all shared texture handles are opened correctly before attempting to use them. r=jrmuizel

4c72e43c5ec2	Matt Woodrow — Bug 1176506 - Don't test texture sharing on WARP devices since it never works and can be detected as a driver reset. r=Bas
So the sanity test actually ran correctly and isn't white... but Firefox itself shows a black screen in the content process. The black screen is e10s only. This is bug 1175366.
See Also: → 1175366
Sorry copy pasted wrong, black screen on e10s sites is bug 1176506.
See Also: 11753661176506
Changes the sanity test to test both layers and video. Previously, if video failed, we would not test layers. In this particular bug, video worked but layers didn't, so I think it'd be good to test both regardless. Also cleans up a bit of leftover cruft from deleting the OS snapshotting code.
Flags: needinfo?(matt.woodrow)
Attachment #8630578 - Flags: review?(dvander)
This part changes the sanity test to load a remote XUL browser once the parent process has loaded the window from the nsWindowWatcher. It creates a XUL browser and loads the content script. The content side loads the actual sanity test, and replies through the message manager with a message once the URI has loaded. Once the parent side receives the message from the content side, it takes the snapshot.

Mostly just looking for review that we're creating browser.xul correctly and correctly sending the messages over the message manager. I tried to use background page thumb XUL component as a guiding post.
Attachment #8630580 - Flags: review?(mconley)
Attachment #8630578 - Flags: review?(dvander) → review+
Comment on attachment 8630580 [details] [diff] [review]
Part 2 - Load sanity test in a remote XUL browser

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

::: toolkit/components/gfx/content/gfxFrameScript.js
@@ +28,5 @@
> +  onStateChange: function (webProgress, req, flags, status) {
> +    if (webProgress.isTopLevel &&
> +        (flags & Ci.nsIWebProgressListener.STATE_STOP) &&
> +        this.isSanityTest(req.name)) {
> +      sendAsyncMessage('gfxSanity::ContentLoaded');

does this guarantee all paints are flushed?
Comment on attachment 8630580 [details] [diff] [review]
Part 2 - Load sanity test in a remote XUL browser

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

Just some nits, but this looks sane.

::: toolkit/components/gfx/SanityTest.js
@@ +127,5 @@
>    canvas: null,
> +  mm: null,
> +
> +  messages: [
> +    "gfxSanity::ContentLoaded",

We generally use single colon delimiters for our pseudonamespacing, but meh

@@ +156,5 @@
> +      case "gfxSanity::ContentLoaded":
> +        this.runSanityTest();
> +        break;
> +    }
> +

Extra newline

@@ +170,5 @@
> +    browser.style.width = PAGE_WIDTH + "px";
> +    browser.style.height = PAGE_HEIGHT + "px";
> +
> +    this.win.document.documentElement.appendChild(browser);
> +    // Have to set the mm after we apend the child

Nit: "append"

::: toolkit/components/gfx/content/gfxFrameScript.js
@@ +1,5 @@
> +const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
> +
> +const contentscript = {
> +  init: function() {
> +    var webNav = docShell.QueryInterface(Ci.nsIWebNavigation);

let, not var throughout

@@ +21,5 @@
> +    if (splitResult.length == 0) {
> +      return false;
> +    }
> +
> +    return splitResult[splitResult.length - 1] == "sanitytest.html";

This is roughly equivalent to:

if (!aUri) {
  return false;
}

return aUri.endsWith("/sanitytest.html");
Attachment #8630580 - Flags: review?(mconley) → review+
Updated with feedback from comment 12. Also now waits for "MozAfterPaint" in the content process after getting a load event as paints are still pending. 

Sorry for the second round of review.
Attachment #8630580 - Attachment is obsolete: true
Attachment #8630762 - Flags: review?(mconley)
Attachment #8630762 - Flags: review?(dvander)
Attachment #8630762 - Flags: review?(dvander) → review+
Comment on attachment 8630762 [details] [diff] [review]
Part 2 - Load sanity test in a remote XUL browser

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

LGTM, thanks!

::: toolkit/components/gfx/content/gfxFrameScript.js
@@ +4,5 @@
> +  domUtils: null,
> +
> +  init: function() {
> +    let webNav = docShell.QueryInterface(Ci.nsIWebNavigation);
> +    let webProgress =  docShell.QueryInterface(Ci.nsIInterfaceRequestor).

Busted consistency - we've got a period at the end of here, followed by getInterface, and we've got .getInterface on line 13. Let's go with line 13's technique.
Attachment #8630762 - Flags: review?(mconley) → review+
Carrying r+, updated to put the period in the right place from comment 14.
Attachment #8630762 - Attachment is obsolete: true
Attachment #8631203 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6eb9c407ca8b
https://hg.mozilla.org/mozilla-central/rev/d4b9c91aa19e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: