Remove default Recv__delete__ implementations if there are any arguments
Categories
(Core :: IPC, enhancement, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox75 | --- | fixed |
People
(Reporter: jld, Assigned: jld)
References
Details
Attachments
(1 file)
Discussion in bug 1612671 turned up a problem with devirtualization (bug 1512990): Recv__delete__
has an empty implementation in the superclass, so trying to “override” with different types the same way as a normal devirtualized Recv
method either shadows it (causing a compiler warning) or, if the superclass method is using
ed, results in overloading and allows the possibility of silently using the wrong method if there's a mistake in the types.
However, we can avoid this problem by generating that method only if there are no arguments to the __delete__
message: in that case the subclass method will always have the same argument types (namely, none), and if there are arguments then it doesn't make much sense to ignore them, and in practice nobody is doing that.
I have a patch for this; it more or less passes Try and I'm mostly convinced that the failures aren't my fault (and if they are then I think that would imply a bug somewhere else, because this shouldn't be a functional change).
Assignee | ||
Comment 1•4 years ago
|
||
Not quite — it seems to cause this apparent permafail in a Fission devtools test, crashing with Assertion failure: IsWindowProxy(obj)
. That shouldn't be possible; I'll try rerunning the job a few times for more data. There are also a few other failures in that run that might not be false positives.
Assignee | ||
Comment 2•4 years ago
|
||
(In reply to Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ from comment #1)
Not quite — it seems to cause [this apparent permafail][fail] in a Fission devtools test, crashing with
Assertion failure: IsWindowProxy(obj)
.
I can't reproduce it locally, and it does seem to happen on my Try run with my patch but not my Try run of the same base m-c revision unmodified; however, there is an existing bug on file for it: bug 1604593. (It also appears to always be the same test case as my failures.)
Also, as far as I can tell nothing currently in tree is using
the inherited Recv__delete__
, which should mean there are no cases where it's overloaded (in contrast to overriding it as-is or hiding it with a differently typed function), which means that it's not possible for this patch to silently cause functional changes.
Comment 3•4 years ago
|
||
I think the IsWindowProxy() thing is already permafailing. At least, I've been needinfo'd on it...
Assignee | ||
Comment 4•4 years ago
|
||
The default method implementations cause problems when trying to
override them with different types in a direct call class.
For the Recv__delete__ case there's a simple solution: omit it if
there are any arguments, because it doesn't make much sense to specify
arguments and then completely ignore them, and the no-arg case isn't a
problem for overriding.
Updated•4 years ago
|
Pushed by jedavis@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ba82dc114a30 Don't generate default `Recv__delete__` if there are args. r=nika
Comment 6•4 years ago
|
||
bugherder |
Description
•