Bug 1536047 Comment 14 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

> Can you please give me a guideline of needs to be done

Generally speaking, see the documentation at https://searchfox.org/mozilla-central/rev/4763b8d576ce52625d245d1ab6d9404ea025b026/build/clang-plugin/CanRunScriptChecker.cpp#6-44

You want to make sure the code is following the rules described there.

> Since this is only seen when compiling in automation and not locally on Windows

Does adding --enable-clang-plugin to your mozconfig work on Windows, by any chance?  That's what automation is doing, to run the analysis at build time.  This worked for me out of the box on Mac, with a lot of pain (had to compile my own clang) on Linux.  Not sure what the Windows situation is...

> If I understand it correctly, one says whether you can call the function from JS and the other whether the function itself can call into JS?

That's correct.

> but we don't control the IDL that declares OnStopRequest so how is that going to work?

Not well-documented (and perhaps not intended, but still true) is that you can annotate a _definition_ with MOZ_CAN_RUN_SCRIPT_BOUNDARY.  That is probably the right path here.  File an m-c bug to annotate the OnStopRequest IDL with [can_run_script], annotate your thing MOZ_CAN_RUN_SCRIPT_BOUNDARY with a comment pointing to the m-c bug.

> We should probably be annotation all the functions annotated with JS_HAZ_CAN_RUN_SCRIPT with MOZ_CAN_RUN_SCRIPT 

That's tracked in bug 1534292, fwiw.  Timeframe: extremely uncertain, if ever.  ;)

> Looks like mFakeThis needs to be turned into a strong reference

Given the structure of the NS_FORWARD_NSIMSGCOMPOSE macro, I think you should be able to do:

  NS_FORWARD_NSIMSGCOMPOSE(RefPtr<JaCppComposeDelegator> fakeThis(mFakeThis); fakeThis->>JaBaseCppCompose::)

or so.  That said, I don't understand how this code is supposed to work at all, at first glance; mFakeThis is refcounted but we're not holding a ref to it.  We just set the raw pointer in the constructor and never null it out.  Looks pretty scary to me.  :(
> Can you please give me a guideline of needs to be done

Generally speaking, see the documentation at https://searchfox.org/mozilla-central/rev/4763b8d576ce52625d245d1ab6d9404ea025b026/build/clang-plugin/CanRunScriptChecker.cpp#6-44

You want to make sure the code is following the rules described there.

> Since this is only seen when compiling in automation and not locally on Windows

Does adding --enable-clang-plugin to your mozconfig work on Windows, by any chance?  That's what automation is doing, to run the analysis at build time.  This worked for me out of the box on Mac, with a lot of pain (had to compile my own clang) on Linux.  Not sure what the Windows situation is...

> If I understand it correctly, one says whether you can call the function from JS and the other whether the function itself can call into JS?

That's correct.

> but we don't control the IDL that declares OnStopRequest so how is that going to work?

Not well-documented (and perhaps not intended, but still true) is that you can annotate a _definition_ with MOZ_CAN_RUN_SCRIPT_BOUNDARY.  That is probably the right path here.  File an m-c bug to annotate the OnStopRequest IDL with [can_run_script], annotate your thing MOZ_CAN_RUN_SCRIPT_BOUNDARY with a comment pointing to the m-c bug.

> We should probably be annotation all the functions annotated with JS_HAZ_CAN_RUN_SCRIPT with MOZ_CAN_RUN_SCRIPT 

That's tracked in bug 1534292, fwiw.  Timeframe: extremely uncertain, if ever.  ;)

> Looks like mFakeThis needs to be turned into a strong reference

Given the structure of the NS_FORWARD_NSIMSGCOMPOSE macro, I think you should be able to do:
```
  NS_FORWARD_NSIMSGCOMPOSE(RefPtr<JaCppComposeDelegator> fakeThis(mFakeThis); fakeThis->>JaBaseCppCompose::)
```
or so.  That said, I don't understand how this code is supposed to work at all, at first glance; mFakeThis is refcounted but we're not holding a ref to it.  We just set the raw pointer in the constructor and never null it out.  Looks pretty scary to me.  :(

Back to Bug 1536047 Comment 14