Devirtualize More Methods/Classes with Final Keywords

NEW
Unassigned

Status

()

defect
P3
normal
2 years ago
4 months ago

People

(Reporter: tjr, Unassigned)

Tracking

(Blocks 3 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 affected)

Details

(Whiteboard: [overhead:noted])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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!)
(Reporter)

Updated

2 years ago
Depends on: 1330608
(Reporter)

Updated

2 years ago
Blocks: 1332682
(Reporter)

Updated

2 years ago
Depends on: 620058
(Reporter)

Updated

2 years ago
Depends on: 1360299
(Reporter)

Updated

2 years ago
Depends on: 1360300
(Reporter)

Updated

2 years ago
Depends on: 1360301
(Reporter)

Updated

2 years ago
No longer depends on: 1360301
(Reporter)

Updated

2 years ago
No longer depends on: 1360300
(Reporter)

Updated

2 years ago
No longer depends on: 1360299
(Reporter)

Comment 1

2 years ago
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
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
(Reporter)

Comment 6

2 years ago
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.
(Reporter)

Updated

a year ago
Blocks: 1415303
(Reporter)

Updated

a year ago
Attachment #8869560 - Attachment is obsolete: true
(Reporter)

Comment 9

a year ago
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.)

Comment 13

a year ago
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 :)
(Reporter)

Updated

a year ago
Blocks: 1443252
Attachment #8955498 - Attachment mime type: text/x-log → text/plain
Attachment #8955501 - Attachment mime type: text/x-log → text/plain
Depends on: 1444175
(Reporter)

Updated

a year ago
Depends on: 1444490
(Reporter)

Updated

a year ago
Depends on: 1446509
(Reporter)

Updated

a year ago
Blocks: 1449288
Whiteboard: [overhead:noted]

Updated

9 months ago
Priority: -- → P3
See Also: → 1517816
You need to log in before you can comment on or make changes to this bug.