931912 - Suppress an exact rooting hazard false positive in nsWindowSH::Finalize

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Terrence Cole [:terrence]]

Attachment: suppress_nsWindowSH_Finalize-v0.diff

Quoting from the patch:
+  // Since this call is virtual, the exact rooting hazard static analysis is
+  // not able to determine that it happens during finalization and should be
+  // ignored. Moreover, the analysis cannot discover and validate the
+  // potential targets of the virtual call to OnFinalize below because of the
+  // indirection through nsCOMMPtr. Thus, we annotate the analysis here so
+  // that it does not report OnFinalize as GCing with |obj| on stack.

At least, this is how I interpret what is going on here. Steve, does the above sound reasonable to you?

Comment 1

[Boris Zbarsky [:bzbarsky]]

s/nsCOMMPtr/nsCOMPtr/ and seems fine.

Comment 3

[Steve Fink [:sfink] [:s:]]

Comment on attachment 823446 [details] [diff] [review]
suppress_nsWindowSH_Finalize-v0.diff

Review of attachment 823446 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay. I'm still thinking the analysis might be able to figure this out from its callers (as in, it should know that finalization cannot GC.) But I haven't looked to see if there are nasty problems with that approach. So this is good.

Comment 4

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a245d66f692

Comment 5

[Terrence Cole [:terrence]]

And backed out for opt bustage by:
https://hg.mozilla.org/integration/mozilla-inbound/rev/83c4706d9fe3

Normally, the only use is |this|. Even when the class is empty, there are implicit constructors and destructors that "run", but since there is also no data in the class |this| is never read or written. It turns out the opt build I did before pushing did have this warning, but for some reason my local build is not respecting warn-as-error. Bummer.

Comment 6

[Terrence Cole [:terrence]]

Attachment: suppress_nsWindowSH_Finalize-v1.diff

https://tbpl.mozilla.org/?tree=Try&rev=8e5e7fc94a7e

Added MOZ_UNUSED. Will flag for review of the new bits if Try is green.

Comment 7

[Terrence Cole [:terrence]]

And of course windows still complains because that technique can't work there. Lets try adding an empty constructor.

https://tbpl.mozilla.org/?tree=Try&rev=21463f4415f2

Comment 8

[Terrence Cole [:terrence]]

That seemed to work better. No new code in mfbt, so no new review needed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b5ffddb8bad

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0b5ffddb8bad