Closed Bug 1338870 Opened 3 years ago Closed 3 years ago

Review use of "super respondsToSelector" in nsCocoaWindow.mm

Categories

(Core :: Widget: Cocoa, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: dmajor, Assigned: mstange)

Details

Attachments

(1 file)

I don't speak Objective-C and merely stumbled across this code, so apologies if I'm missing something.

Please see this code: https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/widget/cocoa/nsCocoaWindow.mm#3191,3211

My understanding of https://developer.apple.com/reference/objectivec/1418956-nsobject/1418583-respondstoselector is that "super respondsToSelector" will always be true in those cases in nsCocoaWindow.mm, since the subclass overrides those methods, so that test is probably not doing what was intended.

Another thing that confuses me is that we're either calling on super or on NSWindow, but this code is in BaseWindow, so super _is_ NSWindow, is it not?
Flags: needinfo?(mstange)
Thanks for spotting this, I had no idea!

(In reply to David Major [:dmajor] from comment #0)
> Another thing that confuses me is that we're either calling on super or on
> NSWindow, but this code is in BaseWindow, so super _is_ NSWindow, is it not?

Well, super is an object whose class is NSWindow. The difference here is that [super frameRectForContentRect:aRect styleMask:aMask] calls the *instance* method on this object ("-[NSWindow frameRectForContentRect:styleMask:]"), and [NSWindow frameRectForContentRect:aRect styleMask:aMask] calls the *class* method ("+[NSWindow frameRectForContentRect:styleMask:]") which doesn't have access to this window object.

The class variant of this method is documented / public, the instance method is undocumented and not guaranteed to be present.
Flags: needinfo?(mstange)
Comment on attachment 8838639 [details]
Bug 1338870 - Don't call [super respondsToSelector:] because it doesn't do what you might think.

https://reviewboard.mozilla.org/r/113476/#review115014
Attachment #8838639 - Flags: review?(spohl.mozilla.bugs) → review+
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/805f47239008
Don't call [super respondsToSelector:] because it doesn't do what you might think. r=spohl
Assignee: nobody → mstange
https://hg.mozilla.org/mozilla-central/rev/805f47239008
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.