Closed Bug 1509562 Opened Last year Closed Last year

Organizational improvements when defining redirections

Categories

(Core :: Web Replay, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: bhackett, Assigned: bhackett)

Details

Attachments

(5 files)

Attached patch patchSplinter Review
There are a couple of annoying things about how redirections are defined:

- Redirections are defined as a flat array, which makes it hard to have different redirections which are installed in different ways, or to (in the future) share sets of redirections between different platforms.

- Redirections are defined via a gigantic macro, which is ugly, breaks some editors (in particular, the editor I use), and makes it hard to diagnose certain compiler errors.

The attached patch does a bunch of reorganization to fix both of these, while avoiding making any functional changes.
Allow platforms to internally manage how their redirections are organized, allowing other parts of the system to access the redirections with NumRedirections() and GetRedirection(n) methods.
Attachment #9027254 - Flags: review?(lsmyth)
Fix a bug exposed by later patches in this bug.  When resetting middleman calls, releasing the resources held via a call can require accessing data from earlier middleman calls.  These calls may have already been deleted, leading to a use-after-free error.  The attached patch deletes the calls in a separate pass to avoid this.
Attachment #9027255 - Flags: review?(lsmyth)
Defining the redirections outside a macro means that we will no longer be able to use the CallEvent_* enum values to refer to the original unredirected function pointer for a given redirection name.  Uses of these enums are pretty limited, and this patch adds an alternative way to refer to the original unredirected function pointer for the 25 or so functions that need this behavior.
Attachment #9027256 - Flags: review?(lsmyth)
Replace the gigantic macro used to define the redirections with a normal static initializer.  This also renames the Middleman_* functions to MM_* for compactness and to more closely mirror the similar RR_* functions.
Attachment #9027257 - Flags: review?(lsmyth)
Attachment #9027254 - Flags: review?(lsmyth) → review+
Attachment #9027255 - Flags: review?(lsmyth) → review+
Attachment #9027256 - Flags: review?(lsmyth) → review+
Attachment #9027257 - Flags: review?(lsmyth) → review+
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f77f0486954
Part 1 - Remove restriction on redirections being a statically defined array, r=lsmyth.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb62b5dc4dbe
Part 2 - Avoid UAF when resetting middleman calls, r=lsmyth.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0efb6f89bd0
Part 3 - Remove dependence on CallEvent_* enum, r=lsmyth.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9f41dd78cfd
Part 4 - Define redirections directly, instead of with a giant macro, r=lsmyth.
You need to log in before you can comment on or make changes to this bug.