1536738 - `this` in methods marked as MOZ_CAN_RUN_SCRIPT is not considered as safe

Status: RESOLVED INVALID

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Foo::MozCanRunScript() {
  mBar->MaybeCanRunScript(this);
}

is considered as not safe. I.e., it seems that the following two code blocks might be considered as different:

Foo::MozCanRunScript() {
  AnotherCanRunScript();
}

Foo::MozCanRunScript() {
  this->AnotherCanRunScript();
}

When MozCanRunScript() is called, the instance of Foo should be grabbed with local RefPtr or something. Therefore, this in MOZ_CAN_RUN_SCRIPT methods should be safe.

Comment 1

[Boris Zbarsky [:bzbarsky]]

>Foo::MozCanRunScript() {
>  mBar->MaybeCanRunScript(this);
>}

This would be considered unsafe because it's using mBar, which might get nulled out by the script running.

We don't seem to have explicit tests for the "this->AnotherCanRunScript()" case, but I just added one and it passes. I'll attach that here, but could you please double-check whether the problem you're actually running into is with "this" or with mBar?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Automated testcase

Comment 3

[Boris Zbarsky [:bzbarsky]]

I'm going to sneak this testcase into some other changes I'm making to the static analysis anyway.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #1)

We don't seem to have explicit tests for the "this->AnotherCanRunScript()" case, but I just added one and it passes. I'll attach that here, but could you please double-check whether the problem you're actually running into is with "this" or with mBar?

Oh, right. I confirmed that ^ is under the "m" of "mBar". Sorry for taking your time.