Bug 1226826 adds some patches to record a log from individual checkerboard instances and dump it to stderr. Instead of dumping them we can save them and create an about:checkerboard page to visualize them and help diagnose what caused them. People can then file bugs with the logs/info, which will provide more actionable info to us.

WIPs so far at https://github.com/staktrace/gecko-dev/commit/444d55680d8a0644863299ff59ff34a5067baeb5 and https://github.com/staktrace/gecko-dev/commit/171763f29d37ecc628fadc9acf3f86ce109b471e

Not really sure who should review the dom/ side so tagging ehsan - feel free to redirect. r? botond for the gfx/ side of things. This patch creates a singleton storage class that will store the checkerboard reports and that DOMWindowUtils can query. This will be used in the second patch to populate about:checkerboard.

Most of the content of about:checkerboard is the page I have in https://github.com/staktrace/rendertrace/ - this will likely change over time as we make changes. For now it's mostly just the minimum we need to be able to visualize checkerboarding instances.

These patches apply on top of those in bug 1238040.

Comment on attachment 8709568 [details] [diff] [review] Part 1 - Expose checkerboard reports to DOMWindowUtils Review of attachment 8709568 [details] [diff] [review]: ----------------------------------------------------------------- r=me ideally with the below fixed (but it's up to you ultimately.) ::: dom/webidl/CheckerboardReport.webidl @@ +5,5 @@ > + */ > + > +/* > + * This file declares data structures used to communicate checkerboard reports > + * from C++ code to about:checkerboard (see bug 1238042). Please say explicitly that these dictionaries are not used anywhere exposed to the Web. @@ +16,5 @@ > +dictionary CheckerboardReport { > + unsigned long severity; > + DOMTimeStamp timestamp; > + DOMString log; > + DOMString reason; So this can only be "severe" or "recent", right? In this case, it is more idiomatic to represent it using an enum, like: enum CheckerboardReason { "severe", "recent" }; dictionary CheckerbaordReport { // ... CheckerboardReason reason; }; This will give you |CheckerboardReason::Severe/Recent| in C++.

Comment on attachment 8709570 [details] [diff] [review] Part 2 - add about:checkerboard Review of attachment 8709570 [details] [diff] [review]: ----------------------------------------------------------------- I'm assuming Botond or someone else will review the actual js code in aboutCheckerboard.js. I haven't... ::: toolkit/components/aboutcheckerboard/content/aboutCheckerboard.xhtml @@ +16,5 @@ > + <button onclick="toggleEnabled()">Toggle it!</button>.</p> > + <table class="listing" cellspacing="0"> > + <tr> > + <th>Most severe checkerboarding reports</th> > + <th>Most recent checkerboarding reports</th> What's up with all of these unlocalized strings? Are you doing this intentionally?

I wasn't sure if there was much value in localizing this page; I was considering it similar in nature to about:memory and about:performance which are also not localized. If you think it's worth localizing I can do it, it's not that much effort on my part, more work for the localizers.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7) > I wasn't sure if there was much value in localizing this page; I was > considering it similar in nature to about:memory and about:performance which > are also not localized. If you think it's worth localizing I can do it, it's > not that much effort on my part, more work for the localizers. Hmm, good question. flod, what do you think? One question may be whether you expect this page to be accessed by non-technical non-English speaking users...

At this point, no, I don't really expect it to be accessed by a non-technical audience.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7) > I wasn't sure if there was much value in localizing this page; I was > considering it similar in nature to about:memory and about:performance which > are also not localized. I agree: about:checkerboard seems like a very technical page, with a limited-audience (we also have about:buildconfig in the same range), I would keep the strings hard-coded. Unrelated to this bug: on the other hand I will probably file a bug for about:performance, that seems to have some value also for non tech users.

(In reply to :Ehsan Akhgari from comment #5) > > Please say explicitly that these dictionaries are not used anywhere exposed > to the Web. > > So this can only be "severe" or "recent", right? In this case, it is more > idiomatic to represent it using an enum, like: > I made these changes locally. Thanks for the tip about enum!

Comment on attachment 8709568 [details] [diff] [review] Part 1 - Expose checkerboard reports to DOMWindowUtils Review of attachment 8709568 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/CheckerboardReport.webidl @@ +14,5 @@ > +// log of the event, and the reason this report was saved (currently either > +// "severe" or "recent"). > +dictionary CheckerboardReport { > + unsigned long severity; > + DOMTimeStamp timestamp; // milliseconds since epoch ::: gfx/layers/apz/src/AsyncPanZoomController.cpp @@ +3125,5 @@ > > mPotentialCheckerboardTracker.CheckerboardDone(); > > if (recordTrace) { > + // if the pref is enabled, also save it in the storage class // this will show up in about:checkerboard ::: gfx/layers/apz/util/CheckerboardEventStorage.h @@ +56,5 @@ > + * Struct that this class uses internally to store a checkerboard report. > + */ > + struct CheckerboardReport { > + uint32_t mSeverity; // if 0, this report is empty > + int64_t mTimestamp; // microseconds since epoch, as returned by JS_Now()

Comment on attachment 8709570 [details] [diff] [review] Part 2 - add about:checkerboard Review of attachment 8709570 [details] [diff] [review]: ----------------------------------------------------------------- This generally looks good. A few notes: - Since about:checkerboard requires chrome privileges, it needs security review [1]. - Having the visualizer (rendertrace) built into the browser isn't essential. We could have about:checkerboard just expose the log, and require the consumer to copy it from there and paste it into rendertrace running as a regular website hosted somewhere. This would significantly decrease the amount of code that would need to be security-reviewed. The rendertrace page can still live in the tree, and thus be hosted on hg.m.o, similar to the reftest analyzer. [1] https://dxr.mozilla.org/mozilla-central/rev/b67316254602a63bf4e568198a5c7d3288a9db27/docshell/base/nsAboutRedirector.cpp#24

Updated per review comments, carrying r+

Updated this patch with review comments. Now all the visualization JS code is loaded into an unprivileged web content iframe from hg.m.o, so it doesn't need to be sec-review'd. The actual privileged about:checkerboard code is very small.

Comment on attachment 8710136 [details] [diff] [review] Part 2 - add about:checkerboard (v2) Review of attachment 8710136 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/aboutcheckerboard/content/aboutCheckerboard.js @@ +20,5 @@ > + reports = dwu.getCheckerboardReports().reports; > + for (var i = 0; i < reports.length; i++) { > + let text = "Severity " + reports[i].severity + " at " + new Date(reports[i].timestamp).toString(); > + let link = document.createElement('a'); > + link.href = 'javascript:showReport(' + i + ')'; This will do the wrong thing (navigate to a blank frame) when you click the link, wouldn't it? Why not just set an onclick handler? @@ +41,5 @@ > +} > + > +function toggleEnabled() { > + Services.prefs.setBoolPref("apz.record_checkerboarding", > + !Services.prefs.getBoolPref("apz.record_checkerboarding")); Does this pref have a default value? If yes, you should consider using clearUserPref() to clear it instead. ::: toolkit/components/aboutcheckerboard/content/aboutCheckerboard.xhtml @@ +24,5 @@ > + <td><ul id="recent"></ul></td> > + </tr> > + </table> > + > + <iframe id="render" src="https://hg.mozilla.org/mozilla-central/raw-file/tip/toolkit/components/aboutcheckerboard/rendertrace.html"></iframe> Instead of mocking with iframes like this, I suggest to use a trick similar to BrowserFeedWriter to expose one extra API to about:checkerboarding and drop the privileges from this about page altogether.

Comment on attachment 8710136 [details] [diff] [review] Part 2 - add about:checkerboard (v2) Review of attachment 8710136 [details] [diff] [review]: ----------------------------------------------------------------- As discussed on IRC, we're going to take the slightly different approach of not giving about:checkerboard chrome privileges at all, but instead giving it access to the specific APIs it needs. Dropping review flag until the patch is updated.

Ok, here we go with the latest agreed-upon version. It's the first time I've written code that binds into JS like this, so it's quite possible I screwed something up totally. It seems to work and passes tests though. There's two classes introduced here: mozilla::layers::CheckerboardEventStorage, which is the singleton c++-only class that holds the reports, and there's mozilla::dom::CheckerboardReportService which is the class that gets bound to JS objects via the WebIDL. I spent a lot of time trying to unify the two but I was unable to, mostly because I think to participate in cycle collection properly the JS wrapper needs to store a per-instance parent object which is incompatible with it being a singleton. Both of these live in the same .h/.cpp file because they're intimately related, and the header has to be exported into mozilla/dom because of how the WebIDL bindings have auto-generated #include statements and assume a particular namespace.

And this is the about:checkerboard page, now running unprivileged, but having access to the CheckerboardReportService object. Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=92a726ebb1c1

Comment on attachment 8710433 [details] [diff] [review] Part 1 - Expose checkerboard reports to JS (v3) Review of attachment 8710433 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/apz/util/CheckerboardReportService.h @@ +109,5 @@ > + > + static already_AddRefed<CheckerboardReportService> > + Constructor(const dom::GlobalObject& aGlobal, ErrorResult& aRv); > + > + CheckerboardReportService(nsISupports* aSupports); Oh I need to make this explicit. I'll do that locally.

Comment on attachment 8710433 [details] [diff] [review] Part 1 - Expose checkerboard reports to JS (v3) Review of attachment 8710433 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/CheckerboardReportService.webidl @@ +36,5 @@ > +interface CheckerboardReportService { > + /** > + * Gets the available checkerboard reports. > + */ > + CheckerboardReports getReports(); Hmm, this is weird. Why aren't you returning a sequence<CheckerboardReport> directly? ::: gfx/layers/apz/util/CheckerboardReportService.cpp @@ +105,5 @@ > + CheckerboardReport& r = mCheckerboardReports[i]; > + if (r.mSeverity == 0) { > + continue; > + } > + dom::CheckerboardReport* report = list.AppendElement(fallible); Why fallible?? @@ +154,5 @@ > + > + // Now check the spec itself > + nsAutoCString spec; > + uri->GetSpec(spec); > + return spec.EqualsLiteral("about:checkerboard"); Do you mind filing a follow-up to unify this function and the one for the browser feed writer? @@ +199,5 @@ > + > +void > +CheckerboardReportService::SetRecordingEnabled(bool aEnabled) > +{ > + gfxPrefs::SetAPZRecordCheckerboarding(aEnabled); I assume this does the right thing in the content process? ::: gfx/layers/apz/util/CheckerboardReportService.h @@ +100,5 @@ > + /** > + * Check if the given page is allowed to access this object via the WebIDL > + * bindings. It only returns true if the page is about:checkerboard. > + */ > + static bool AllowAccess(JSContext* aCtx, JSObject* aGlobal); Nit: It's more idiomatic to call this function Enabled or some such. See |grep Enabled dom/webidl| for examples.

(In reply to :Ehsan Akhgari from comment #21) > > + CheckerboardReports getReports(); > > Hmm, this is weird. Why aren't you returning a sequence<CheckerboardReport> > directly? Ah good point, I can do that. I had some reason for leaving it like this but it can't have been very good because I've forgotten it, and it seems silly now. > > + dom::CheckerboardReport* report = list.AppendElement(fallible); > > Why fallible?? Seemed like that's what everybody else did, as of bug 968520. And honestly we can handle a fallible allocation here so why not. > @@ +154,5 @@ > > + return spec.EqualsLiteral("about:checkerboard"); > > Do you mind filing a follow-up to unify this function and the one for the > browser feed writer? Yup, will do. > @@ +199,5 @@ > > + gfxPrefs::SetAPZRecordCheckerboarding(aEnabled); > > I assume this does the right thing in the content process? Probably not, but none of this code is intended to be used in the content process, because the checkerboard reports themselves are only available in the parent process. I can change the constructor guard (the one that checks the URL) to only allow using this class in the parent process. > ::: gfx/layers/apz/util/CheckerboardReportService.h > > + static bool AllowAccess(JSContext* aCtx, JSObject* aGlobal); > > Nit: It's more idiomatic to call this function Enabled or some such. See > |grep Enabled dom/webidl| for examples. Ok, will rename.

Comment on attachment 8710434 [details] [diff] [review] Part 2 - Add about:checkerboard (v3) Review of attachment 8710434 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/aboutcheckerboard/content/aboutCheckerboard.js @@ +1,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +"use strict"; I'll let Botond go over this file. :-)

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #22) > > > + dom::CheckerboardReport* report = list.AppendElement(fallible); > > > > Why fallible?? > > Seemed like that's what everybody else did, as of bug 968520. And honestly > we can handle a fallible allocation here so why not. Handling allocation failures in cases where web content cannot control the size of the allocation and the size is not a linear function of the input size is pointless, since either the allocation will never fail, or we'll OOM crash soon afterwards if we're really running out of memory. Please switch to non-fallible allocation here. > > @@ +199,5 @@ > > > + gfxPrefs::SetAPZRecordCheckerboarding(aEnabled); > > > > I assume this does the right thing in the content process? > > Probably not, but none of this code is intended to be used in the content > process, because the checkerboard reports themselves are only available in > the parent process. I can change the constructor guard (the one that checks > the URL) to only allow using this class in the parent process. Sorry, I managed to confuse myself here. You don't have URI_CAN_LOAD_IN_CHILD so this will never run in the content process anyways. Please ignore me!

I might as well do this in this bug. Part 0 goes first, I'll rebase part 1 to use this helper.

Comment on attachment 8710433 [details] [diff] [review] Part 1 - Expose checkerboard reports to JS (v3) Review of attachment 8710433 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/apz/src/AsyncPanZoomController.cpp @@ +35,5 @@ > #include "mozilla/ReentrantMonitor.h" // for ReentrantMonitorAutoEnter, etc > #include "mozilla/StaticPtr.h" // for StaticAutoPtr > #include "mozilla/Telemetry.h" // for Telemetry > #include "mozilla/TimeStamp.h" // for TimeDuration, TimeStamp > +#include "mozilla/dom/CheckerboardReportService.h" // for CheckerboardEventStorage Can we include it under mozilla/layers in this location? Or would that just make things more confusing?

(In reply to Botond Ballo [:botond] from comment #26) > Can we include it under mozilla/layers in this location? Or would that just > make things more confusing? If your concern is somebody going looking in dom/ for that header, I can add a comment there that says it is really in apz/util. Would that be sufficient? I could also just include it directly from apz/util but I don't know which is better.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #27) > (In reply to Botond Ballo [:botond] from comment #26) > > Can we include it under mozilla/layers in this location? Or would that just > > make things more confusing? > > If your concern is somebody going looking in dom/ for that header Yeah. For me, Eclipse figures out where it is, but vim users may not be as fortunate :) >, I can add > a comment there that says it is really in apz/util. Would that be > sufficient? Yep, thanks.

Comment on attachment 8710707 [details] [diff] [review] Part 0 - Extract a helper function Review of attachment 8710707 [details] [diff] [review]: ----------------------------------------------------------------- Fantastic! Thank you.

Comment on attachment 8710434 [details] [diff] [review] Part 2 - Add about:checkerboard (v3) Review of attachment 8710434 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! I tried this out in a local build, and it's pretty cool functionality to have. Please note that I'm not an experienced reviewer of JS code, so my apologies if I overlooked anything, and if I asked for anything that's un-idiomatic in JS please feel free to ignore me. ::: docshell/base/nsAboutRedirector.cpp @@ +46,5 @@ > }, > + { > + "checkerboard", "chrome://global/content/aboutCheckerboard.xhtml", > + nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT | > + nsIAboutModule::ALLOW_SCRIPT Do we want web content to be able to link to about:checkerboard? We can control this via the nsIAboutModule::MAKE_UNLINKABLE flag. I don't feel strongly either way, just wanted to bring your attention to it. ::: toolkit/components/aboutcheckerboard/content/aboutCheckerboard.js @@ +17,5 @@ > + link.href = 'javascript:showReport(' + i + ')'; > + link.textContent = text; > + let bullet = document.createElement('li'); > + bullet.appendChild(link); > + document.getElementById(reports[i].reason).appendChild(bullet); Can we check that the getElementById() call succeeded, and show an error somewhere if it didn't? (Feel free to ignore this if you think it's overkill.) @@ +38,5 @@ > + updateEnabled(); > +} > + > +function showReport(index) { > + document.getElementById('trace').value = reports[index].log; Consider caching document.getElementById('trace')? @@ +73,5 @@ > +// for each matching line, tokenizes on whitespace and ignores all tokens prior to > +// RENDERTRACE. Additional info can be included at the end of the line, and will be > +// displayed but not parsed. Allowed syntaxes: > +// <junk> RENDERTRACE <timestamp> rect <color> <x> <y> <width> <height> [extraInfo] > +// <junk> RENDERTRACE <timestamp> vector <color> <x> <y> [extraInfo] Who uses "vector"? CheckerboardEvent doesn't appears to. If no one uses it, I'd prefer removing it (and the corresponding code in renderFrame()); we can always add it later if we need to. @@ +86,5 @@ > + > + var charPos = 0; > + var lastLineLength = 0; > + var lines = document.getElementById('trace').value.split(/\r|\n/); > + var rendertraceIndex = 0; This variable appears unused. @@ +98,5 @@ > + > + var tokens = lines[i].split(/\s+/); > + var j = 0; > + // skip tokens until RENDERTRACE > + while (j < tokens.length && tokens[j++] != "RENDERTRACE"); "// empty loop body" so we know it's deliberate @@ +119,5 @@ > + // timestamp hasn't changed use, so update the previous object > + destIndex--; > + } else { > + // clone a new copy of the last frame and update timestamp > + renderData.push(JSON.parse(JSON.stringify(renderData[destIndex - 1]))); Is 'JSON.parse(JSON.stringify(x))' the idiomatic way to deep-copy an object in JS? @@ +124,5 @@ > + renderData[destIndex].timestamp = timestamp; > + } > + > + switch (tokens[j++]) { > + case "fillrect": "fillrect" is not mentioned in the allowed syntax, and CheckerboardEvent doesn't generate it either. @@ +131,5 @@ > + log("Error parsing line: " + lines[i]); > + continue; > + } > + > + var rect = { type: tokens[j-1] }; 'type' is probably superfluous if we don't have "fillrect" @@ +132,5 @@ > + continue; > + } > + > + var rect = { type: tokens[j-1] }; > + var id = tokens[j++]; s/id/color? @@ +139,5 @@ > + rect.y = parseFloat(tokens[j++]); > + rect.width = parseFloat(tokens[j++]); > + rect.height = parseFloat(tokens[j++]); > + rect.dataStart = charPos; > + rect.dataEnd = charPos + lines[i].length; Why not just store the string segment here? @@ +297,5 @@ > +} > + > +function togglePlay() { > + if (playing) { > + clearInterval(timerId); If you pull out the body of this branch into a pause() function, some places that call togglePlay() could call pause() instead, which I think would be more readable. ::: toolkit/components/aboutcheckerboard/content/aboutCheckerboard.xhtml @@ +10,5 @@ > + <link rel="stylesheet" href="chrome://global/content/aboutCheckerboard.css" type="text/css"/> > + <script type="text/javascript;version=1.8" src="chrome://global/content/aboutCheckerboard.js"></script> > + </head> > + > + <body onload="onLoad()"> Might it be worth having a "Reload" button, so that if you checkerboard while you already have about:checkerboard open in a tab, you can click it to pick up the new reports? @@ +11,5 @@ > + <script type="text/javascript;version=1.8" src="chrome://global/content/aboutCheckerboard.js"></script> > + </head> > + > + <body onload="onLoad()"> > + <p>Checkerboard recording is <span id="enabled" style="color: red">undetermined</span>. Why is this style not in the css file? @@ +28,5 @@ > + <hr/> > + > + <div id="player"> > + <div id="controls"> > + <button onclick="reset(true)">&#171;</button> Please add the action names in comments, so someone reading the code doesn't have to figure out what "&#171;" renders as to understand what this is intended to do.

(In reply to Botond Ballo [:botond] from comment #30) > Do we want web content to be able to link to about:checkerboard? We can > control this via the nsIAboutModule::MAKE_UNLINKABLE flag. I don't feel > strongly either way, just wanted to bring your attention to it. It might be useful to link to, I figure we can keep it unless there's a reason not to. > Can we check that the getElementById() call succeeded, and show an error > somewhere if it didn't? (Feel free to ignore this if you think it's > overkill.) I do think it's a bit overkill. The web console should display an error message already if this happens. > > Consider caching document.getElementById('trace')? Done > Who uses "vector"? CheckerboardEvent doesn't appears to. > > If no one uses it, I'd prefer removing it (and the corresponding code in > renderFrame()); we can always add it later if we need to. Removed > > + var rendertraceIndex = 0; > > This variable appears unused. Removed > > + while (j < tokens.length && tokens[j++] != "RENDERTRACE"); > > "// empty loop body" so we know it's deliberate Done > > Is 'JSON.parse(JSON.stringify(x))' the idiomatic way to deep-copy an object > in JS? I'm not sure, but it seems to be one of the recommended methods on http://stackoverflow.com/questions/122102/what-is-the-most-efficient-way-to-clone-an-object if you are not using JQuery. > > + case "fillrect": > > "fillrect" is not mentioned in the allowed syntax, and CheckerboardEvent > doesn't generate it either. Removed > > + var rect = { type: tokens[j-1] }; > > 'type' is probably superfluous if we don't have "fillrect" Yup, removed > > + var id = tokens[j++]; > > s/id/color? Changed > > + rect.dataEnd = charPos + lines[i].length; > > Why not just store the string segment here? I was a little worried about the string duplication overhead but it's probably not worth worrying about. I updated this to store the string segment in a dataText field and got rid of dataStart/dataEnd. > If you pull out the body of this branch into a pause() function, some places > that call togglePlay() could call pause() instead, which I think would be > more readable. Good call, done. > > + <body onload="onLoad()"> > > Might it be worth having a "Reload" button, so that if you checkerboard > while you already have about:checkerboard open in a tab, you can click it to > pick up the new reports? Mm.. I figure people can use the browser reload for that. It will lose your state in the current animation, but if you want to view the new checkerboard event you'll lose that anyway. > > + <p>Checkerboard recording is <span id="enabled" style="color: red">undetermined</span>. > > Why is this style not in the css file? I modify it using .style.color in some JS. The ".style" field on an element specifically refers to the style attribute on an element. If I stick it in the CSS file, setting .style.color = 'red' wouldn't update the color in the CSS file, it would add a second CSS rule to this element. Depending on the cascade rules the file one might have precedence in some cases (e.g. if it's flagged as !important). > > + <button onclick="reset(true)">&#171;</button> > > Please add the action names in comments, so someone reading the code doesn't > have to figure out what "&#171;" renders as to understand what this is > intended to do. Done.

New build-only try push to make sure more non-unified bustage didn't sneak in since my last try push. https://treeherder.mozilla.org/#/jobs?repo=try&revision=1fbcbf18fe4e

How about one that actually includes my patches... https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f6ccce49420

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd49c46ef0fd https://hg.mozilla.org/integration/mozilla-inbound/rev/2a392116c7ca https://hg.mozilla.org/integration/mozilla-inbound/rev/e165659ef803

https://hg.mozilla.org/mozilla-central/rev/bd49c46ef0fd https://hg.mozilla.org/mozilla-central/rev/2a392116c7ca https://hg.mozilla.org/mozilla-central/rev/e165659ef803