We need this to test fennec and firefox.next. The basic plan of attack will be - add code to detect whether <browser remote=true/false> is configured. In fennec we can just read the browser.tabs.remote pref. If true, reftests will run in a "remote" mode. (Not to be confused with the existing reftest.remote mode, or firefox's remote mode. Sigh.) - separate the reftest.js code that interacts with test pages from the code that drives test runs and snapshots+compares renderings - for remote mode, load the test-interaction code into the content process and drive it using IPC messages The rest should work automagically. It should be possible in theory to use the in-process message manager to keep the same implementation for both remote=true and false.

(In reply to comment #0) > It should be possible in theory to use the > in-process message manager to keep the same implementation for both remote=true > and false. This seems like the solution with the least overall complexity, as long as it doesn't slow down running in-process reftests exponentially.

It had better not, or we're boned all over the place ;). But it won't: same-process messages boil down to essentially posting a runnable. Should be faster than setTimeout(foo, 0), in fact.

Totally unscientific early perf comparison, on the reftest-sanity suite in-tree reftest.js: real 0m6.211s user 0m4.890s sys 0m0.520s reftest.js + reftest-content.js: real 0m6.330s user 0m5.020s sys 0m0.450s Numbers fairly stable across runs, within ~100ms real time. So looks like we're in the range of noise-to-few-%-slowdown, on my machine.

reftest+reftest-content is working in-process, but it's now relying on a couple of things that won't work OOP. It's passing all the tests and the synchronization issues have been tentatively worked out, so a more realistic perf comparison is feasible (in-tree reftest.js) REFTEST INFO | Successful: 5470 (5456 pass, 14 load only) REFTEST INFO | Total canvas count = 11 real 12m40.654s user 11m41.350s sys 0m36.990s (reftest+reftest-content) REFTEST INFO | Successful: 5470 (5456 pass, 14 load only) REFTEST INFO | Total canvas count = 8 real 13m16.815s user 12m5.050s sys 0m42.240s (I don't understand why we end up with fewer total canvases with the new code ... hm.) So a ~5% slowdown. Most of this is likely due to a new drawWindow() needed to force a layer-tree update for the OOP case. We could use a different tactic when running in-process, if need be.

(In reply to comment #4) > So a ~5% slowdown. Most of this is likely due to a new drawWindow() needed to > force a layer-tree update for the OOP case. We could use a different tactic > when running in-process, if need be. That doesn't affect its ability to test invalidation, does it?

In my WIP, no. It's only doing this sync before InitCurrentCanvasWithSnapshot(). I haven't figured out yet what do for UpdateCurrentCanvasForInvalidation() with shadow layers.

(In reply to comment #5) > (In reply to comment #4) > > So a ~5% slowdown. Most of this is likely due to a new drawWindow() needed to > > force a layer-tree update for the OOP case. We could use a different tactic > > when running in-process, if need be. > > That doesn't affect its ability to test invalidation, does it? Actually, I'd like to understand this better ... for which class of tests is it important that we use UpdateCurrentCanvasForEvent() instead of taking another whole-window snapshot? There's no way in the current framework to determine if an invalidation is "minimal", in some sense, right? (I.e., the results would be the same if the invalidated region were the entire window.) Is the update-for-event path just to make the tests run faster?

No, it also finds bugs where we didn't invalidate a large enough rectangle.

OK, I think we're all on the same page then. As long as the force-retained-layer-update drawWindow doesn't invalidate any additional content, it seems like we're OK. I wrote it in such a way that, to my knowledge, it won't.

With a special case for in-process updates, we're back down to REFTEST INFO | Successful: 5470 (5456 pass, 14 load only) REFTEST INFO | Total canvas count = 9 real 12m51.020s user 11m57.350s sys 0m37.400s So back down to a ~1% slowdown, possibly within noise (didn't do multiple trials, for obvious reasons).

That sounds fine. More than fine.

OK, added another in-process tweak and we're at REFTEST INFO | Successful: 5470 (5456 pass, 14 load only) REFTEST INFO | Total canvas count = 7 real 12m46.690s user 11m44.620s sys 0m44.240s <1% slowdown isn't going to be noticeable (if it's not noise).

Bug 617152 needs to land before this or it's going to be a major pain to rebase bug 617152. Much easier to rebase this.

Got the in-process hacks straightened out (with another hack), and out-of-process (apparently, mostly) working now. Early classes of failures are - printing tests (?!?). I have no idea what's involved here, might need to selectively disable for the time being. - failures from focus issues - two interesting rendering failures - failure from frame issue (might be a reftest-ipc bug) - assertion failures from code trying to set prefs in the content process All but the last class look like legitimate bugs, though out-of-process printing may need to be reclassified as a deferred feature.

Print tests are failing because we end up using a temporary layer manager when painting, due to going into a "page mode" described as // XXX Page mode is only partially working; it's currently used for // reftests that require a paginated context (Dunno if this comment is still accurate.) We could certainly support print tests with a fair bit of work. It's difficult to justify this effort currently when fennec doesn't (can't) support printing and ff5 is a ways off. The key question is, will disabling print tests for <browser remote> cause us to miss bugs that would bite more generally? (Non-printing.) If not, it's hard for me to justify this effort right now. The assertion failures are all bug 617525.

Bug 583976 (should) cover inputField.focus() in tests.

I realize too that we're probably going to run into bug 617653 in reftest-ipc, though I haven't gotten that far yet. I don't think it's wise to block landing reftest-ipc on some relatively major platform features that we would want to use reftest-ipc to test when implemented. So, I'd like to propose the following approach for dealing with reftests that are broken because of a NYI-big-feature X - add |sandbox.XWorks = !gBrowserIsRemote;| to reftest.js - mark tests of X as |skip-if(!XWorks)|. (Would prefer to use random-if, but X being NYI might result in a timeout.) - developers implementing feature X would set |sandbox.XWorks = true;| - when X passes all tests, the developer(s) would run |sed -i s/skip-if\(\!XWorks\) //| before landing X This has the downside of a large, non-trivial initial annotation burden, but I'm signed up for that. There's also a hazard for authors of new tests of feature X who only test in-process locally in that if they don't know about |skip-if(!XWorks)|, they'll fail on tryserver (or m-c). (Hopefully the test will be near others that use the same annotation, to supply a hint.) And there's of course the problem of authors adding this annotation to tests that aren't actually failing because X is NYI and thereby masking real bugs. The upside is, we can land reftest-ipc without adding temporary (possibly fragile) hacks to the reftest harness or the platform, can enable reftest-ipc on tinderbox without wasting time on "regressions" of NYI features, and have a clear exit strategy for when X is implemented. Thoughts?

I doubt you'll run into bug 617653. We don't reftest the contents of a <select> dropdown, because we can't. I think you could automatically skip reftest-print tests in remote mode. You won't lose any coverage you care about.

(In reply to comment #18) > I doubt you'll run into bug 617653. We don't reftest the contents of a <select> > dropdown, because we can't. > Do we have reftests for pages that include <select>s? (Closed.) > I think you could automatically skip reftest-print tests in remote mode. You > won't lose any coverage you care about. Sure, it's possible. Let me run through the remaining tests to see if there are any more major NYI features causing failures.

BTW, note another big NYI feature causing failures and timeouts is focus() of input fields. We can't (sanely) work around that in the reftest harness.

(In reply to comment #19) > Do we have reftests for pages that include <select>s? (Closed.) Yes.

(Blocking on bug 616414 because it's a potential source of intermittent failures.)

I've got all the failures (that don't also fail on my desktop in-process) down to (0) leaking gfxASurfaces (again!). Arghh. (1) focus. Worst offender; causes many tests to hang from not getting focus (2) printing (3) intermittent failures that depend on timing (fixed by setTimeout(1000) in the right place in the reftest harness) (1) is a major feature covered by bug 583976, which is made more complicated by fennec's workarounds. (2) is a major feature that's way off the radar atm. I'm hoping that (3) is due to reftest.js issues that bug 617152 will fix. I'll recheck after that's ready. I don't expect real fixes for (1) or (2) in the near future. It's possible for us to skip printing tests in the reftest harness, with some relatively hairy code, but it's not possible (sanely anyway) for us to skip tests that rely on focus. So I'm going to re-propose comment 17, more concretely: - add sandbox.bug617632=gBrowserIsRemote for printing - add sandbox.bug583976=gBrowserIsRemote for focus - mark failing tests appropriately Objections?

(Quick note: bug 618176 doesn't really hard-block this because I have a workaround. But don't tell mrbkap.)

Mostly for archival purposes. Passes tests that don't otherwise fail for me in-process (modulo tests disabled OOP) (modulo timing). Will split for review when bug 617152 lands.

The goal of the logic split was to move all the rendering+waiting code into reftest-content.js, and keep the existing test-driving+image-comparison+etc. code in reftest.js. The split was mostly mechanical. This patch is mostly deleting the test-driving logic, which stays in reftest.js in part 2. The nontrivial parts of this patch were (1) for remote content, ensuring that drawWindow of the <window> in chrome saw the right <browser> content. This reduced to forcing a shadow-layers transaction in content just before requesting a drawWindow in chrome. This is done with a drawWindow(<0,0, 1,1>, USE_WIDGET_LAYERS) in content. (2) for remote content, drawWindow in chrome *must* USE_WIDGET_LAYERS or nothing will be drawn for the <browser remote>, since it can't (won't) synchronously repaint content. The comments in DoDrawWindow imply that there are cases in which the reftest <window> is resized in a way that precludes USE_WIDGET_LAYERS. This must happen on (only?) tinderbox or I suspect we wouldn't care. That might be a big problem for reftest-ipc. (3) for local content, neither issue above applies, so the harness skips the synchronization code and does a synchronous drawWindow wrt reftest-content.js to avoid slowing down existing tests (see comment 12). !USE_WIDGET_LAYERS will continue to work for <browser remote=false> even if it's broken for remote=true. (4) for remote content, async vs. sync drawWindow requests don't matter for correctness, so async is used to let the test continue in parallel with the drawWindow(s). (5) dynamically creating the reftest <browser>. We don't know the value of browser.tabs.remote when reftest.xul is loaded, so don't know whether to set browser.remote=true/false then. Flipping browser.remote after browser instantiation isn't something anyone cares about, and probably doesn't work properly, so instead the harness dynamically creates the browser with the right "remote" attribute. Some distracting patch hunks come from message-manager-script discrepancies wrt old reftest.js - |content| in reftest-content.js == |gBrowser.contentWindow| in old reftest.js - accessing some interfaces requires a different incantation - setTimeout/clearTimeout aren't globals so content.set/clear are used instead

(r? to roc since he's doing part 1. Let me know if dbaron should review this, possibly in addition.) Mostly deleting content-interaction code. Noteworthy changes - chrome-process assertions also count towards total for remote content - incidentally fixed dump-sandbox spamming since it's always the same for all manifests now - incidentally fixed warning for nsIGfxInfo

Ehsan: just looking for a sanity check that disabling these tests won't cause us to miss bad non-focus-related bugs.

(In reply to comment #27) > Created attachment 500357 [details] [diff] [review] > part 2: Remove bits of reftest harness that interact with content, hook up > "IPC" from content script, and add reftest-ipc target I should also call out the fact that |sandbox.focusWorks$bug583976| was added to reftest.js to allow easier testing of the focus patches. It's not possible (well, sanely ;) ) to skip these tests from within the harness itself.

(I'll push these three patches folded together.)

(In reply to comment #26) > (2) for remote content, drawWindow in chrome *must* USE_WIDGET_LAYERS > or nothing will be drawn for the <browser remote>, since it can't > (won't) synchronously repaint content. The comments in DoDrawWindow > imply that there are cases in which the reftest <window> is resized > in a way that precludes USE_WIDGET_LAYERS. This must happen on > (only?) tinderbox or I suspect we wouldn't care. That might be a big > problem for reftest-ipc. I found out accidentally on tryserver that this happens during the linux reftests, but obviously doesn't on my desktop. Sigh. Is that a known bug? Some infra thing?

Hmmm if we had in-process displayport, we could work around this. Doesn't seem worth the effort though if there's a simpler infra fix.

Crap. Happens on mac too.

Also disables tests that are failing for known reasons (bug 622224 looks worst). Don't think I need review on this but I'd be happy if someone looked it over.

(In reply to comment #33) > Crap. Happens on mac too. Turns out I accidentally pulled a Bas on 10.6, somehow: all the canvases came out blank, so all the != tests failed. So not apparently related to window/screen size. Confusingly, all the reftests passed on 10.5 :S.

Wait no, scratch that. Was looking at an opt build, which apparently doesn't report javascript errors.

Comment on attachment 500356 [details] [diff] [review] part 1: Split out parts of reftest harness that must interact directly with content and add synchronization logic for OOP content This is a pretty massive rewrite of the reftest harness, but this part looks OK to me.

Comment on attachment 500357 [details] [diff] [review] part 2: Remove bits of reftest harness that interact with content, hook up "IPC" from content script, and add reftest-ipc target There's a potential race in UpdateCurrentCanvasForInvalidation in the sense that there's no guarantee each DoDrawWindow is going to use results from the same frame. But I guess that's OK since all we care about in the end is the final result.

You can look in any Tinderbox reftest logs to see which machines are able to get USE_WIDGET_LAYERS currently. There are bugs filed to get all the machines suitably configured. One thing that would be pretty cool --- for tests that we know need focus, if the reftest window doesn't have focus, skip them. That would make it easier for people to run reftests while doing something else on the machine.

You should check with dbaron to see how much review he wants to do here.

(In reply to comment #37) > This is a pretty massive rewrite of the reftest harness, but this part looks OK > to me. There's a lot of diff hunks, but the total number of substantial changes were surprisingly small, at least IMHO. (In reply to comment #38) > There's a potential race in UpdateCurrentCanvasForInvalidation in the sense > that there's no guarantee each DoDrawWindow is going to use results from the > same frame. I don't quite follow this. The logic was pretty carefully written so that we will drawWindow the same pixels starting from SendUpdateCurrent as we would in UpdateCurrent on tip. Do you see a bug there or did you have something else in mind? (In reply to comment #39) > You can look in any Tinderbox reftest logs to see which machines are able to > get USE_WIDGET_LAYERS currently. There are bugs filed to get all the machines > suitably configured. I found this out by accident through a reftest.js bug. No linux machines USE_WIDGET_LAYERS across the board; commented in that bug. The mac 10.6 machines were unexpectedly not getting it either; filed bug 622229. Unfortunately I only really care about linux right now, but c'est la guerre. > > One thing that would be pretty cool --- for tests that we know need focus, if > the reftest window doesn't have focus, skip them. That would make it easier for > people to run reftests while doing something else on the machine. How would I determine if the window has focus? (Or I guess too if it can be focused?) I do now have a list of tests that fail when focus is broken.

(In reply to comment #40) > You should check with dbaron to see how much review he wants to do here. dbaron?

Minor status update: the crashtest run timed out on two try pushes, only on debug-windows-5.2, on different tests but near the same place in the test run. I couldn't repro the timeout locally in a winxp or win7 VM. So, I suspect the tests are now running slower and timing out. I suspect the way I ported the logging code is to blame, will poke. I think we're going to need a workaround for bug 622224 before landing this. That bug is worse than I thought. Will post another patch here and qfold with the others.

The patches here were rebased over bug 621339, but that was mechanical so I don't think re-review is needed.

(In reply to comment #41) > I don't quite follow this. The logic was pretty carefully written so that we > will drawWindow the same pixels starting from SendUpdateCurrent as we would in > UpdateCurrent on tip. Do you see a bug there or did you have something else in > mind? Ah, so layer transactions can't arrive while the script is running? right.

The drawWindow requests originating from OOP content are strongly sequenced before any future shadow-layer transactions, yes. (Through IPDL in-order message delivery.) But, as you say, if we screw this up we should see test failures ;).

Bug 622224 covers implementing setTimeout/clearTimeout globally, for frame scripts. Without this, the previous iteration of this patch used the content-window's setTimeout/clearTimeout. However, AIUI from smaug, using the content-window causes timeouts to be dropped on page navigation. The reftest harness depends on being able to set up timeouts independent of the page being tested, and numerous tests failed when that was broken. So this patch works around bug 622224 by implementing very simple setTimeout()/clearTimeout() within the frame script, using nsITimer.

Same as the last version, except with the tests that failed before the bug 622224 workaround back enabled.

Looked OK on try, so should be ready to land after review unless anyone objects.

The removed deps have been worked around, don't block this landing anymore.

(In reply to comment #28) > Created attachment 500359 [details] [diff] [review] > part 3: Disable reftests that fail when focus is broken (e.g. in <browser > remote>) > > Ehsan: just looking for a sanity check that disabling these tests > won't cause us to miss bad non-focus-related bugs. When is focusWorks$... set to true? And why are you trying to win the worst named variable ever award? Just call it focusWorks! ;-)

(In reply to comment #51) > (In reply to comment #28) > > Created attachment 500359 [details] [diff] [review] > > part 3: Disable reftests that fail when focus is broken (e.g. in <browser > > remote>) > > > > Ehsan: just looking for a sanity check that disabling these tests > > won't cause us to miss bad non-focus-related bugs. > > When is focusWorks$... set to true? + // Focus and input events don't yet work out-of-the-box for + // out-of-process content, and implementing them is a major + // feature complicated by fennec's needs. It's not possible for + // the reftest harness to automatically skip these tests. + // Implementation is bug 583976. + sandbox.focusWorks$bug583976 = !gBrowserIsRemote; > And why are you trying to win the worst > named variable ever award? Just call it focusWorks! ;-) Heh. Fine with me. I named it like this for easier copying+pasting+eventual-removal. Other places we disable like this, we do |skip load foo.html # bug XXX| but there were a lot of places disabled for broken focus.

Comment on attachment 500359 [details] [diff] [review] part 3: Disable reftests that fail when focus is broken (e.g. in <browser remote>) I think instead of this patch, I'll add code that gets us to what roc suggested (don't run focus tests when the window can't be focused), but I'll file a followup for detecting has-focus/can-focus. And of course for now, can-focus is overridden to false by gBrowserIsRemote=true. I think this might be best implemented by a |focus| keyword available to reftest manifests, so that one would write focus == blah.html tuva.html Sound OK?

Moved blockers for reftest-ipc to bug 623613. They don't block landing the harness changes for same-process content only.

I don't believe this needs review either.

(In reply to comment #53) > Comment on attachment 500359 [details] [diff] [review] > part 3: Disable reftests that fail when focus is broken (e.g. in <browser > remote>) > > I think instead of this patch, I'll add code that gets us to what roc suggested > (don't run focus tests when the window can't be focused), but I'll file a > followup for detecting has-focus/can-focus. And of course for now, can-focus > is overridden to false by gBrowserIsRemote=true. > > I think this might be best implemented by a |focus| keyword available to > reftest manifests, so that one would write > focus == blah.html tuva.html > > Sound OK? Sounds good to me!

There's no shadow-layers support in either D3D backend yet.

I think this should block (really, blocking-fennec but we don't have that flag here) because - these are our only automated tests of shadow-layer rendering - there are shadow-layer features and bugs that can only be tested with this setup; e.g. bug 593243 and bug 605618 - it's a burden for platform devs to build fennec to test patches. This reftest-ipc suite is available in normal --enable-ipc FF builds on all Tier Is. On the practical side, it's all ready to go modulo reviews.

Just to be clear, is this blocking- wrt fennec or desktop? I wanted blocking-fennec? but don't have the flag.

Comment on attachment 501701 [details] [diff] [review] part 3: Add reftest support for marking tests as needing focus, and flag those that are known to fail without focus Could you call the new keyword needs-focus instead of just focus? > >- // Focus and input events don't yet work out-of-the-box for >- // out-of-process content, and implementing them is a major >- // feature complicated by fennec's needs. It's not possible for >- // the reftest harness to automatically skip these tests. >- // Implementation is bug 583976. >- sandbox.focusWorks$bug583976 = !gBrowserIsRemote; >- This seems to be removing code that isn't in the tree. Another patch in your queue, perhaps? >+ // FIXME/bug 623625: determine if the window is focused and/or try >+ // to acquire focus if it's not. >+ return true; Could you add a comment that we shouldn't add anything to the non-remote mode that would return false, since then we could lose testing coverage due to problems on the test machines? What's the deal with the "XXX" comment related to script test? r=dbaron with that

Comment on attachment 501207 [details] [diff] [review] part 4: Work around bug 622224 r=dbaron, I guess, although I don't know much about this file to begin with

(In reply to comment #62) > Comment on attachment 501701 [details] [diff] [review] > part 3: Add reftest support for marking tests as needing focus, and flag those > that are known to fail without focus > > Could you call the new keyword needs-focus instead of just focus? > Sure. > > > >- // Focus and input events don't yet work out-of-the-box for > >- // out-of-process content, and implementing them is a major > >- // feature complicated by fennec's needs. It's not possible for > >- // the reftest harness to automatically skip these tests. > >- // Implementation is bug 583976. > >- sandbox.focusWorks$bug583976 = !gBrowserIsRemote; > >- > > This seems to be removing code that isn't in the tree. Another patch in your > queue, perhaps? > Yes, part 2 above. > >+ // FIXME/bug 623625: determine if the window is focused and/or try > >+ // to acquire focus if it's not. > >+ return true; > > Could you add a comment that we shouldn't add anything to the non-remote mode > that would return false, since then we could lose testing coverage due to > problems on the test machines? > Hmmm ... I wonder if we should have a "force-focus" mode then, that tinderbox would use, but defaulting to checking the actual window. I guess that's a discussion for bug 623625. I'll add your note to this comment. > What's the deal with the "XXX" comment related to script test? > I wasn't sure whether it made sense for script tests to require focus. It doesn't harm anything though, and maybe there's a use case for it. I'll toast that comment.

(In reply to comment #63) > Comment on attachment 501207 [details] [diff] [review] > part 4: Work around bug 622224 > > r=dbaron, I guess, although I don't know much about this file to begin with Thanks. I wanted to be sure this was a suitable placeholder impl of setTimeout.

Carrying over r+'s.

Can't request blocking-fennec or approval on patches from the Testing product.

Comment on attachment 508655 [details] [diff] [review] Rollup of bug 615386, parts 1-6. Teach the reftest harness about <browser remote> Comment 0 and comment 60 describe what we gain by landing this code. Also see the bugs that this bug blocks; this is the last dep for running automated tests of out-of-process rendering. There's not really any risk ... either these patches pass the existing tests or they don't. This obviously can't land until it passes all tests, so that point is moot. The worst I can imagine is that, because this will change test timing a bit, it might expose new intermittent failures in buggy tests.

Comment on attachment 508655 [details] [diff] [review] Rollup of bug 615386, parts 1-6. Teach the reftest harness about <browser remote> a2.0=dbaron

crashtest-ipc is now failing some content/xslt/crashtests/ because of ###!!! ASSERTION: This will screw up our existing presentation: '!mPresShell && !mWindow', file /home/cjones/mozilla/mozilla-central/layout/base/nsDocumentViewer.cpp, line 2400 but that doesn't block this from landing. Will investigate later. Everything else looks good, locally.

Rebasing on bug 624279 was the first nontrivial one. It adds a boolean return from InitCurrentCanvasWithSnapshot(), which crosses "processes" in the updated harness. I fixed this in the dumbest way possible, just forwarded the return value back to reftest-content.

Remaining issues will block bug 623613. http://hg.mozilla.org/mozilla-central/rev/c1523d3f7841

Missed one http://hg.mozilla.org/mozilla-central/rev/c618b7b138fb Long story.