IPDL failures may no longer cause reftest failures

NEW
Unassigned

Status

Testing
Reftest
a year ago
11 months ago

People

(Reporter: kats, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Created attachment 8833301 [details]
Log snippet showing IPDL failures with reftests still passing

At some point it looks IPDL failures were changed to not trigger crashes. This means that reftests that run into IPDL failures may not fail. In particular, a couple of reftests added in bug 1298218 relied on APZ, which isn't present in non-e10s. So the tests should have failed in non-e10s, but they didn't. See the attached log snippet for the reftest output from an example run.

I'm pretty sure the IPDL not-crashing change was intentional, but I don't know if this was foreseen as a side-effect. :dvander, do you have any thoughts on what the intended behaviour here is?
^
Flags: needinfo?(dvander)
This  definitely wasn't intended. I think it's a regression either from bug 792652 or bug 1314254.
Flags: needinfo?(wmccloskey)
Actually, I guess I'm wrong. Unless the top-level protocol implements ProcessingError, it seems that we don't do anything when a message handler returns false. Do you think we should change this David? The difficulty in such cases is with properly generating crash reports if we do want to kill the other process.
Flags: needinfo?(wmccloskey)
I can't convince myself that it's worth doing, the crash report will be very misleading. The tests should probably be structured in a way that fails when APZ is not functionally present. But maybe we could have a testing pref that makes ProcessingError always crash or something.
Flags: needinfo?(dvander)
In this case I can probably check the return value of [1] and crash explicitly if it's false. But if you're going to make it crash during tests or during debug builds or something then I can wait.

[1] http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/dom/base/nsDOMWindowUtils.cpp#2564
Oh, I wasn't planning on doing it :) It'd probably take a while to flag all the new crashes that are introduced.
You need to log in before you can comment on or make changes to this bug.