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

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mchang, Assigned: mchang)

Tracking

41 Branch
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 2

3 years ago
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.
(Assignee)

Updated

3 years ago
Blocks: 1178447
(Assignee)

Comment 3

3 years ago
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)
(Assignee)

Comment 4

3 years ago
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?
(Assignee)

Comment 6

3 years ago
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
(Assignee)

Comment 7

3 years ago
Created attachment 8628495 [details]
Black screen + sanity test

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.
(Assignee)

Updated

3 years ago
See Also: → bug 1175366
(Assignee)

Comment 8

3 years ago
Sorry copy pasted wrong, black screen on e10s sites is bug 1176506.
See Also: bug 1175366bug 1176506
(Assignee)

Comment 9

3 years ago
Created attachment 8630578 [details] [diff] [review]
Part 1 - Test both layers and video

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)
(Assignee)

Comment 10

3 years ago
Created attachment 8630580 [details] [diff] [review]
Part 2 - Load sanity test in a remote XUL browser

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+
(Assignee)

Comment 13

3 years ago
Created attachment 8630762 [details] [diff] [review]
Part 2 - Load sanity test in a remote XUL browser

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)
(Assignee)

Updated

3 years ago
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+
(Assignee)

Comment 16

3 years ago
Created attachment 8631203 [details] [diff] [review]
Part 2 - Load sanity test in a remote XUL browser

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
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.