Open Bug 1332680 Opened 4 years ago Updated 2 years ago

Devirtualize More Methods/Classes with Final Keywords

Categories

(Core :: General, defect, P3)

defect

Tracking

()

Tracking Status
firefox53 --- affected

People

(Reporter: tjr, Unassigned)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [overhead:noted])

Attachments

(2 files, 1 obsolete file)

Repeat the process in #1047696 to add final keywords. Eliminate vtable lookups and a layer of indirection and also reduce the number of targets for UAF exploitation. (Performance and security gains in one!)
We do gcc builds for Linux. So we don't need MinGW to do this, we just need to get the warnings from our GCC builds and do something about them!
No longer depends on: 1330608
After discussions in SF, the path forward for this is to add -lto and rerun the analysis, and see if we can get the build to complete and give us data.
Okay, I got a successfull LTO build, and extracted out these warnings. I've attached the raw output, and I'm going to paste some high-hit ones here that aren't in third party code:

> 483 /dom/base/nsGlobalWindowInner.h:206:7: warning: Declaring type 'struct nsGlobalWindowInner' final would enable devirtualization of 483 calls
> 143 /dom/base/nsGlobalWindowOuter.h:164:7: warning: Declaring type 'struct nsGlobalWindowOuter' final would enable devirtualization of 143 calls
> 70 /netwerk/cache2/CacheFileIOManager.h:46:7: warning: Declaring type 'struct CacheFileHandle' final would enable devirtualization of 70 calls
> 69 /netwerk/cache2/CacheFileIOManager.h:262:7: warning: Declaring type 'struct CacheFileIOManager' final would enable devirtualization of 69 calls


> 603 /dom/promise/Promise.cpp:66:0: warning: Declaring method 'Release' final would enable devirtualization of 603 calls
> 433 /media/webrtc/signaling/src/jsep/JsepSessionImpl.h:151:0: warning: Declaring method 'GetTransceivers' final would enable devirtualization of 433 calls
> 379 /xpcom/threads/AbstractThread.cpp:197:1: warning: Declaring method 'Release' final would enable devirtualization of 379 calls
> 249 /xpcom/threads/AbstractThread.cpp:197:1: warning: Declaring method 'Release' final would enable devirtualization of 249 calls
> 207 /xpcom/threads/AbstractThread.cpp:197:1: warning: Declaring method 'AddRef' final would enable devirtualization of 207 calls
> 189 /layout/base/nsPresContext.cpp:439:0: warning: Declaring method 'Release' final would enable devirtualization of 189 calls
> 154 /dom/file/Blob.cpp:45:0: warning: Declaring method 'Release' final would enable devirtualization of 154 calls
> 141 /obj-firefox/ipc/ipdl/PContentChild.cpp:4712:0: warning: Declaring method 'GetIPCChannel' final would enable devirtualization of 141 calls
> 124 /xpcom/threads/AbstractThread.cpp:197:1: warning: Declaring method 'AddRef' final would enable devirtualization of 124 calls
> 121 /dom/base/nsGlobalWindowInner.cpp:4487:0: warning: Declaring method 'GetExistingListenerManager' final would enable devirtualization of 121 calls
> 118 /dom/base/nsGlobalWindowInner.cpp:4476:0: warning: Declaring method 'GetOrCreateListenerManager' final would enable devirtualization of 118 calls
> 116 /dom/promise/Promise.cpp:65:0: warning: Declaring method 'AddRef' final would enable devirtualization of 116 calls
> 102 /dom/media/MediaStreamTrack.cpp:199:0: warning: Declaring method 'Release' final would enable devirtualization of 102 calls
> 100 /dom/media/MediaStreamTrack.cpp:199:0: warning: Declaring method 'Release' final would enable devirtualization of 100 calls
> 95 /layout/base/nsPresContext.cpp:438:0: warning: Declaring method 'AddRef' final would enable devirtualization of 95 calls
> 83 /image/imgRequestProxy.cpp:99:0: warning: Declaring method 'Release' final would enable devirtualization of 83 calls
smaug might be interested in looking at the list. There's a lot of DOM stuff in comment 9, and he hates virtual calls. :)
It looks like we may need new variants of the ISUPPORTS decl macros that have final in them. (I hadn't realized you could declare individual methods final, but it makes sense.)
I am glad this warning seems useful.
If you do LTO+PGO build you will get the warnings sorted by the actual number of executions of the devirtualized calls :)
Attachment #8955498 - Attachment mime type: text/x-log → text/plain
Attachment #8955501 - Attachment mime type: text/x-log → text/plain
Depends on: 1444175
Whiteboard: [overhead:noted]
Priority: -- → P3
See Also: → 1517816
You need to log in before you can comment on or make changes to this bug.