Closed Bug 1615155 Opened 4 years ago Closed 4 years ago

Remove default Recv__delete__ implementations if there are any arguments

Categories

(Core :: IPC, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla75
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 usinged, 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).

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.

Priority: P1 → P2

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

Priority: P2 → P1

I think the IsWindowProxy() thing is already permafailing. At least, I've been needinfo'd on it...

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.

Attachment #9126844 - Attachment description: Bug 1615155 - Don't generate default Recv__delete__ if there are args. → Bug 1615155 - Don't generate default `Recv__delete__` if there are args.
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
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: