1444416 - MOZ_CAN_RUN_SCRIPT doesn't work at all with references.

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

The following test fails:

MOZ_CAN_RUN_SCRIPT void test2_parent8() {
  RefCountedBase* t = new RefCountedBase;
  (*t).method_test(); // expected-error {{arguments must all be strong refs or parent parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument)}}
}

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1444416: Handle references in the "can run script" checker.

Review commit: https://reviewboard.mozilla.org/r/226506/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226506/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1444416: Make the diagnostic point to the bogus argument.

It's much easier to find that way, specially for functions with tons of
argument.

Review commit: https://reviewboard.mozilla.org/r/226516/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226516/

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1444416: Make bindings generate appropriate MOZ_KnownLive wrappers.

Review commit: https://reviewboard.mozilla.org/r/226590/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226590/

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8957623 [details]
Bug 1444416: Handle references in the "can run script" checker.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226506/diff/1-2/

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8957628 [details]
Bug 1444416: Make the diagnostic point to the bogus argument.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226516/diff/1-2/

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8957667 [details]
Bug 1444416: Make bindings generate appropriate MOZ_KnownLive wrappers.

https://reviewboard.mozilla.org/r/226590/#review232460

::: dom/bindings/Codegen.py:7424
(Diff revision 1)
>                  a.type.isDOMString()):
>                  arg = CGWrapper(arg, pre="NonNullHelper(", post=")")
> +
> +            # If it's an object, let the static analysis know it's alive for
> +            # the duration of the call.
> +            if a.type.isObject() or a.type.isGeckoInterface():

Why do you need the isObject() check?  I don't think it makes sense, because MOZ_KnownLive only applies to refcounted stuff.  So just the isGeckoInterface() check, please.

r=me with that.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8957667 [details]
Bug 1444416: Make bindings generate appropriate MOZ_KnownLive wrappers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226590/diff/1-2/

Comment 8

[Nika Layzell [:nika] (ooo, ni? for response)]

Comment on attachment 8957623 [details]
Bug 1444416: Handle references in the "can run script" checker.

https://reviewboard.mozilla.org/r/226506/#review232896

LGTM, thanks :-)

Comment 9

[Nika Layzell [:nika] (ooo, ni? for response)]

Comment on attachment 8957628 [details]
Bug 1444416: Make the diagnostic point to the bogus argument.

https://reviewboard.mozilla.org/r/226516/#review232898

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6416568ad06b
Handle references in the "can run script" checker. r=mystor
https://hg.mozilla.org/integration/autoland/rev/4d8dcb700d09
Make the diagnostic point to the bogus argument. r=mystor
https://hg.mozilla.org/integration/autoland/rev/e1e268860e38
Make bindings generate appropriate MOZ_KnownLive wrappers. r=bz

Comment 11

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/6416568ad06b
https://hg.mozilla.org/mozilla-central/rev/4d8dcb700d09
https://hg.mozilla.org/mozilla-central/rev/e1e268860e38