Closed Bug 1536047 Opened 5 years ago Closed 5 years ago

Port latest changes to MOZ_CAN_RUN_SCRIPT from bug 1533293

Categories

(MailNews Core :: General, enhancement)

Product:
MailNews Core
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 68.0

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 5 obsolete files)

1536047-MOZ_CAN_RUN_SCRIPT.patch - WIP
6.97 KB, patch
Details | Diff | Splinter Review
bug1536047_disable_clang_plugin.patch
873 bytes, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
1536047-switch-off-js-account.patch
6.33 KB, patch
Details | Diff | Splinter Review
1536047-Windows.patch (v2)
4.40 KB, patch
Details | Diff | Splinter Review
1536047-re-enable-js-account.patch
1.40 KB, patch
BenB
: review+
Details | Diff | Splinter Review
1536047-revert-nsMsgCompose.patch
5.89 KB, patch
Details | Diff | Splinter Review

/builds/worker/workspace/build/src/comm/mailnews/compose/src/nsMsgCompose.cpp:755:9: error: functions marked as MOZ_CAN_RUN_SCRIPT can only be called from functions also marked as MOZ_CAN_RUN_SCRIPT
/builds/worker/workspace/build/src/comm/mailnews/compose/src/nsMsgCompose.cpp:757:9: error: functions marked as MOZ_CAN_RUN_SCRIPT can only be called from functions also marked as MOZ_CAN_RUN_SCRIPT
/builds/worker/workspace/build/src/comm/mailnews/compose/src/nsMsgCompose.cpp:932:9: 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)
/builds/worker/workspace/build/src/comm/mailnews/compose/src/nsMsgCompose.cpp:932:9: error: functions marked as MOZ_CAN_RUN_SCRIPT can only be called from functions also marked as MOZ_CAN_RUN_SCRIPT

Boris and Emilio, we're seeing frequent bustage due to changes in MOZ_CAN_RUN_SCRIPT. Can you please give me a guideline of needs to be done.

Since this is only seen when compiling in automation and not locally on Windows, it's a particularly cumbersome process to fix this bustage.

We have a few MOZ_CAN_RUN_SCRIPT in C-C now:
https://searchfox.org/comm-central/search?q=MOZ_CAN_RUN_SCRIPT&case=true&regexp=false&path=mailnews%2F
and here:
https://searchfox.org/comm-central/search?q=can_run_script&case=true&regexp=false&path=mailnews%2F

I can't even see the M-C bug that caused it in this range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e0861be8d6c0&tochange=9421b477d67c

Flags: needinfo?(emilio)
Flags: needinfo?(bzbarsky)

Found it, plenty of new MOZ_CAN_RUN_SCRIPT in the three changesets in bug 1533293.

Summary: Port latest changes to MOZ_CAN_RUN_SCRIPT from bug ??? → Port latest changes to MOZ_CAN_RUN_SCRIPT from bug 1533293
Attached patch 1536047-MOZ_CAN_RUN_SCRIPT.patch (obsolete) — — Splinter Review

Wild guess:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3f51772111a25cf4b93f88d29ec918573d5bb779

No idea whether [can_run_script] and [noscript] are compatible:
https://hg.mozilla.org/try-comm-central/rev/047aa0b40d0f926dde7c83b56cbc7ecff602fd13#l1.12

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?

See bug 1534575 comment #7 for Ben's take on this.

Assignee: nobody → jorgk

I do wonder why it isn't showing except for on automation. I just rebuild locally on linux and it went fine. Is there some flag that enables this check?

See bug 1534575 comment #7. Ben looked into it.

(In reply to Jorg K (GMT+1) from comment #0)

Boris and Emilio, we're seeing frequent bustage due to changes in MOZ_CAN_RUN_SCRIPT. Can you please give me a guideline of needs to be done.

Since this is only seen when compiling in automation and not locally on Windows, it's a particularly cumbersome process to fix this bustage.

You need to build with --enable-clang-plugin to see these. Generally what needs to be done is:

  • Annotate any caller to a MOZ_CAN_RUN_SCRIPT function as MOZ_CAN_RUN_SCRIPT.
  • Ensure callers to MOZ_CAN_RUN_SCRIPT functions hold a strong reference to their arguments.
Flags: needinfo?(emilio)

OK, I'll try this locally. As you can see, I had to stick MOZ_CAN_RUN_SCRIPT_FOR_DEFINITION onto QuotingOutputStreamListener::OnStopRequest since that calls convertAndLoadComposeWindow. That brings me to the question: What has to happen in an IDL file. I already had to add it here:
https://hg.mozilla.org/comm-central/rev/f79b60fb833f19ca091c9f209f40ad453c991546#l1.12
but we don't control the IDL that declares OnStopRequest so how is that going to work?

That's exactly happened here now:
/builds/worker/workspace/build/src/comm/mailnews/compose/src/nsMsgCompose.cpp:2468:30: error: functions marked as MOZ_CAN_RUN_SCRIPT cannot override functions that are not marked MOZ_CAN_RUN_SCRIPT

I'm not sure what went wrong here now:
/builds/worker/workspace/build/src/comm/mailnews/compose/src/nsMsgCompose.cpp:4606:15: error: functions marked as MOZ_CAN_RUN_SCRIPT cannot override functions that are not marked MOZ_CAN_RUN_SCRIPT

And sorry about the ignorance, what is a "strong reference"?

Flags: needinfo?(bzbarsky) → needinfo?(emilio)

Regarding the error and 932, you should keep a strong reference to the editor, so:

https://searchfox.org/comm-central/rev/cb9576cec8db2271362a937bc6d1703afa044433/mailnews/compose/src/nsMsgCompose.cpp#932

Should become:

nsCOMPtr<nsIEditor> editor(m_editor);
editor->...

I don't think you need to change the OnStopRequest IDL file, but ICBW, boris would know.

Re. BuildBodyMessageAndSignature, it should probably be marked as CAN_RUN_SCRIPT.

A strong reference is a RefPtr / nsCOMPtr variable on the stack.

Flags: needinfo?(emilio)

Yes, we definitely have the dilemma that we're calling a MOZ_CAN_RUN_SCRIPT function from OnStopRequest :-(

2:49.10 c:/mozilla-source/comm-central/comm/mailnews/compose/src/nsMsgCompose.cpp(3011,7): error: functions marked as MOZ_CAN_RUN_SCRIPT can only be called from functions also marked as MOZ_CAN_RUN_SCRIPT
2:49.10 compose->ConvertAndLoadComposeWindow(mCitePrefix,
2:49.10 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2:49.10 c:/mozilla-source/comm-central/comm/mailnews/compose/src/nsMsgCompose.h(165,5): note: caller function declared here
2:49.10 NS_DECL_NSIREQUESTOBSERVER
2:49.10 ^
2:49.10 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include/nsIRequestObserver.h(47,14): note: expanded from macro 'NS_DECL_NSIREQUESTOBSERVER'
2:49.10 NS_IMETHOD OnStopRequest(nsIRequest *aRequest, nsresult aStatusCode) override;
2:49.10 ^
2:49.14 3 errors generated.

If I put this

MOZ_CAN_RUN_SCRIPT_FOR_DEFINITION NS_IMETHODIMP
QuotingOutputStreamListener::OnStopRequest(nsIRequest *request, nsresult status)

back then I get:
0:24.61 JS_HAZ_CAN_RUN_SCRIPT NS_IMETHOD OnStopRequest(nsIRequest *aRequest, nsresult aStatusCode) = 0;
0:24.61 ^
0:24.61 c:/mozilla-source/comm-central/comm/mailnews/compose/src/nsMsgCompose.cpp(4606,15): error: functions marked as MOZ_CAN_RUN_SCRIPT cannot override functions that are not marked MOZ_CAN_RUN_SCRIPT

So that seems like a lost cause.

Flags: needinfo?(bzbarsky)

Ah, I see... We should probably be annotation all the functions annotated with JS_HAZ_CAN_RUN_SCRIPT with MOZ_CAN_RUN_SCRIPT as well. That may take a bit, let me look into that.

You should be able to work around it by moving OnStopRequest to call something like OnStopRequestInternal, and annotate that one with MOZ_CAN_RUN_SCRIPT_BOUNDARY.

This fixesmailnews/compose/src/nsMsgCompose.cpp but still lots of errors in mailnews/jsaccount/src/:

0:15.02 c:/mozilla-source/comm-central/comm/mailnews/jsaccount/src/JaCompose.h(70,32): 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)
0:15.02 NS_FORWARD_NSIMSGCOMPOSE(mFakeThis->JaBaseCppCompose::)

Looks like mFakeThis needs to be turned into a strong reference, no idea how with all that macro magic. I have to take a break.

Attachment #9051646 - Attachment is obsolete: true

--disable-clang-plugin doesn't appear to be a thing. What matters is ENABLE_CLANG_PLUGIN

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=38adf24a130758759f7fbb512d9d79927c90255e

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. :(

Flags: needinfo?(bzbarsky)

Thanks for all the comments. Yes, --enable-clang-plugin works on Windows. I tried your suggestion

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

and that doesn't compile:
0:11.24 c:/mozilla-source/comm-central/comm/mailnews/jsaccount/src/JaCompose.h(70,62): error: expected '(' for function-style cast or type construction
0:11.24 NS_FORWARD_NSIMSGCOMPOSE(RefPtr<JaCppComposeDelegator> fakeThis(mFakeThis);
0:11.24 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
0:11.24 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\nsIMsgCompose.h(562,126): note: expanded from macro 'NS_FORWARD_NSIMSGCOMPOSE'

That's most likely due to the tricky macro expansion that doesn't take to statements.

This disabling seems to have worked on linux.

Attachment #9051745 - Flags: review?(jorgk)

Does it help if you do:

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

?

Comment on attachment 9051745 [details] [diff] [review]
bug1536047_disable_clang_plugin.patch

How would I tell whether this works on Windows? The try run
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=38adf24a130758759f7fbb512d9d79927c90255e
busted Mach but that might be related to the other hunks.

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

Does it help if you do:

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

Sadly not.

Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/58d5b2195fc8
Port bug 1533293: Add some MOZ_CAN_RUN_SCRIPT to nsMsgCompose.cpp. rs=bustage-fix
https://hg.mozilla.org/comm-central/rev/063e8f6bc499
Temporarily disable MOZ_CAN_RUN_SCRIPT checking. r=jorgk

I've landed that now since our merges are happening soon and it would be nice to merge something that actually compiles.

Comment on attachment 9051678 [details] [diff] [review]
1536047-MOZ_CAN_RUN_SCRIPT.patch - WIP

That fixes the issues in compose, Ben, I'll leave the JS Account macro magic to you, you've already done a good job here:
https://hg.mozilla.org/comm-central/rev/93daa9a87d52

I could swear I had already disabled JS Account once since I couldn't get it to compile, but now I can't find any trace of it.

For the record, it's removing it from mailnews/moz.build and removing all references in mailnews/build/nsMailModule.cpp.
Attachment #9051678 - Flags: review?(benc)
Comment on attachment 9051745 [details] [diff] [review]
bug1536047_disable_clang_plugin.patch

Haha, fixes Linux but not Windows. On Windows you're now seeing the JS Account errors.
Attachment #9051745 - Flags: review?(jorgk) → review+

I've done a bit of digging. NS_FORWARD_NSIMSGCOMPOSE in defined in nsIMsgCompose.h which is generated from the IDL:

#define NS_FORWARD_NSIMSGCOMPOSE(_to) \
  NS_IMETHOD Initialize(nsIMsgComposeParams *aParams, mozIDOMWindowProxy *aWindow, nsIDocShell *aDocShell) override { return _to Initialize(aParams, aWindow, aDocShell); } \
  NS_IMETHOD SetDocumentCharset(const char * charset) override { return _to SetDocumentCharset(charset); } \

We see how the _to argument is used. Looks like we can't squeeze our
RefPtr<JaCppComposeDelegator> fakeThis(mFakeThis); fakeThis->JaBaseCppCompose::
in there.

BenB, can you check how to get this going again? If BenC doesn't find a solution here, I will have to [edit: disable] JS Account for the sake of compiling TB. We're also planning on doing a TB 67 beta sometime soon now.

Flags: needinfo?(benc)
Flags: needinfo?(ben.bucksch)

Oh, it's got the "return" bit in there, I see.

So I think you have a few options:

  1. Hand-code the relevant bits instead of using the macro. Basically hand-expand the macro. It's a bit annoying, because there are lots of methods on nsIMsgCompose.

  2. Try writing it like this:

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

or if that compiles but fails the MOZ_CAN_RUN_SCRIPT analysis, then like this:

  NS_FORWARD_NSIMSGCOMPOSE(MOZ_KnownLive(RefPtr<JaCppComposeDelegator>(mFakeThis))->JaBaseCppCompose::)

with a comment about how the MOZ_KnownLive is a workaround for the analysis not realizing the nsCOMPtr has stack lifetime. And maybe file a bug on the analysis for the latter.

None of the options in No. 2 compiles :-(

None of the options in No. 2 compiles :-(

OK, with what errors?

Sorry, it compiles but fails the analysis.

2.1 says:
0:13.33 c:/mozilla-source/comm-central/comm/mailnews/jsaccount/src/JaCompose.h(49,40): 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)
0:13.33 NS_FORWARD_NSIMSGCOMPOSE(DELEGATE_JS(mJsIMsgCompose, mMethods, mCppBase)->)
0:13.34 ^

Umm, that's a different error now, moved to line 49.

Attached patch jsacc.patch (obsolete) — — Splinter Review

Umm, this still doesn't compile, I get:

0:09.80 In file included from c:/mozilla-source/comm-central/comm/mailnews/jsaccount/src/JaCompose.cpp:7:
0:09.80 c:/mozilla-source/comm-central/comm/mailnews/jsaccount/src/JaCompose.h(50,55): error: expected unqualified-id
0:09.80 (DELEGATE_JS(mJsIMsgCompose, mMethods, mCppBase)->))

Oh, well, yes. I was talking about the mFakeThis usage.

For the DELEGATE_JS usage, how did you write it, exactly? I'd expect you would need something like this:

  NS_FORWARD_NSIMSGCOMPOSE(nsCOMPtr<nsIMsgCompose>(DELEGATE_JS(mJsIAbDirectory, mMethods, mCppBase))->)

or if that is still failing the analysis (per latter part of comment 27), then:

  NS_FORWARD_NSIMSGCOMPOSE(MOZ_KnownLive(nsCOMPtr<nsIMsgCompose>(DELEGATE_JS(mJsIAbDirectory, mMethods, mCppBase)))->)

Sorry, Boris, I did a silly copy/paste, trying your version now.

You can't store the result of DELEGATE_JS in a RefPtr<JaCppComposeDelegator>, because it's totally not that type. It's either a nsCOMPtr<nsIMsgCompose> from mJsIMsgCompose or a nsCOMPtr<nsIMsgCompose> from mCppBase...

Attached patch jsacc.patch (obsolete) — — Splinter Review

Indeed, as I said, silly copy/paste error.

error: no matching function for call to 'MOZ_KnownLive' :-(

Attachment #9051813 - Attachment is obsolete: true

error: no matching function for call to 'MOZ_KnownLive' :-(

Did you include "mozilla/StaticAnalysisFunctions.h" ?

I did before and it gave the same error. Including it now, I also get:

0:11.61 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\mozilla/StaticAnalysisFunctions.h(35,29): note: candidate template ignored: could not match 'T ' against 'nsCOMPtr<nsIMsgCompose>'
0:11.61 static MOZ_ALWAYS_INLINE T MOZ_KnownLive(T* ptr) {

Hmm. Does it help to add an rvalue overload of MOZ_KnownLive (i.e. add a version that takes T&& ref and returns std::forward<T>(ref) to return a T&&)?

BenB, can you check who to get this going again?

I should checked who?

I will have to JS Account

Parse error: "JS Account" is not a verb?

What's your exact problem that you (still, after bz's responses) need solved?

Definition of MOZ_CAN_RUN_SCRIPT: https://hg.mozilla.org/mozilla-central/rev/37761497b6c1

Based on that definition, a lot of TB functions would need this flag, because JsAccount allows core TB functions to be implemented in JS, so their base classes need that flag, and all callers of these functions need that flag, which probably means that half of TB needs it, because most of TB will at some point call mail folder functions or send mail.

bz, JsAccount is a feature of Thunderbird that allows a mail account type (like IMAP and SMTP) to be implemented in JavaScript.

Flags: needinfo?(ben.bucksch)

Right, so if/when bug 1534292 happens that will become an issue.

But for the moment, the problem is just that Thunderbird is calling into editor methods that can run script and that those are annotated with MOZ_CAN_RUN_SCRIPT. The fix on the thunderbird side is to add MOZ_CAN_RUN_SCRIPT and/or MOZ_CAN_RUN_SCRIPT_BOUNDARY annotations sufficient to make things compile, then ideally have a long-term plan for getting rid of the MOZ_CAN_RUN_SCRIPT_BOUNDARY bits. And separately, it may be desirable to start annotating the JsAccount bits as [can_run_script] in IDL and fixing stuff piecemeal instead of waiting for bug 1534292 to break the world. ;)

Attached patch jsacc.patch (obsolete) — — Splinter Review

Here's another variation, I'm calling it a day. Over to the night shift.

Sorry, multiple typos that turned into grammar errors:
BenB, can you check how to get this going again? If BenC doesn't find a solution here, I will have to disable JS Account for the sake of compiling TB.

Comment on attachment 9051678 [details] [diff] [review]
1536047-MOZ_CAN_RUN_SCRIPT.patch - WIP

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

My take on the MOZ_ macros (reading https://searchfox.org/mozilla-central/source/mfbt/Attributes.h) is that usually they should be applied at the function declaration rather than the definition...
I could be wrong on that, but that's my assumption for the comments below.

(I'm syncing and building right now, but then I'll look at the jsaccount stuff. I can try a version of this patch with the macros moved to the header files too if you'd like)

I'll leave the r? for now - if I'm wrong then I can flip it to r+ :-)

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +612,5 @@
>      }
>    }
>  }
>  
> +MOZ_CAN_RUN_SCRIPT_FOR_DEFINITION NS_IMETHODIMP

Should be MOZ_CAN_RUN_SCRIPT on declaration in nsMsgCompose.h instead of here?

@@ +1635,5 @@
>  //  (it did the loadURI that triggered editor creation)
>  // It is called from JS after editor creation
>  //  (loadURI is done in JS)
> +MOZ_CAN_RUN_SCRIPT_FOR_DEFINITION NS_IMETHODIMP
> +nsMsgCompose::InitEditor(nsIEditor* aEditor, mozIDOMWindowProxy* aContentWindow)

Should be MOZ_CAN_RUN_SCRIPT on declaration in nsMsgCompose.h instead of here?

@@ +2472,5 @@
> +  return OnStopRequestInternal(request,status);
> +}
> +
> +MOZ_CAN_RUN_SCRIPT_BOUNDARY nsresult
> +QuotingOutputStreamListener::OnStopRequestInternal(nsIRequest *request, nsresult status)

Should be MOZ_CAN_RUN_SCRIPT_BOUNDARY on declaration in nsMsgCompose.h instead of here?

@@ +4609,5 @@
>    PR_Free(preopen);
>    return NS_OK;
>  }
>  
> +MOZ_CAN_RUN_SCRIPT nsresult

Should be on declaration in nsMsgCompose.h instead of here?

::: mailnews/compose/src/nsMsgCompose.h
@@ +175,5 @@
>      NS_IMETHOD AppendToMsgBody(const nsCString &inStr);
>  
>  private:
>      virtual ~QuotingOutputStreamListener();
> +    nsresult OnStopRequestInternal(nsIRequest *request, nsresult status);

should have MOZ_CAN_RUN_SCRIPT_BOUNDARY here on OnStopRequestInternal declaration?

Sorry, but the functions/methods are defined in the IDL and don't appear in the .h file. The only one where the comment applies is OnStopRequestInternal.

Let's not worry about the fine print here right now. If the JS Account stuff can't be made to compile within in the next 10 hours, I will disable it.

Flags: needinfo?(benc)

Just to be clear, if the goal is just "get stuff compiling ASAP", then I don't understand all the work being done here. Just find the places that are calling the functions that are now MOZ_CAN_RUN_SCRIPT in m-c, annotate those callers with MOZ_CAN_RUN_SCRIPT_BOUNDARY, make those specific calls comply with the MOZ_CAN_RUN_SCRIPT rules, check that in, and file a followup bug to do more work in terms of moving the MOZ_CAN_RUN_SCRIPT_BOUNDARY frontier further out, etc.

Basically, every place you have MOZ_CAN_RUN_SCRIPT_BOUNDARY is a potential security bug, but it's not any worse than it was yesterday in terms of the actual security situation; now you just know about them...

Couldn't even get the static analysis running locally today.
Had a bit of a poke about but my understanding of the attributes and mozilla-can-run-scripts test is still a bit lacking.
I thought that if we tagged all the delegate function implementation with MOZ_CAN_RUN_SCRIPT_BOUNDARY, then they'd be OK calling the real MOZ_CAN_RUN_SCRIPT functions, but no.

Maybe disable the jsaccount stuff and I'll have another crack at it tomorrow?

I need to read through the mozilla-can-run-scripts code and get a proper grasp of what it's doing.
Also, where are the NS_FORWARD_* macros generated? They're not in the .idl file, so it seems to be part of the .idl compilation, but it seems like and awfully specific feature...

Ben, I'd really like to understand what's confusing about the MOZ_CAN_RUN_SCRIPT setup so I can improve the documentation. I'm generally available during work hours US/Eastern and can talk on IRC or video or whatever would work best for you if you'd like...

I need to read through the mozilla-can-run-scripts code and get a proper grasp of what it's doing.

I assume the documentation I linked above does not really answer your questions?

Also, where are the NS_FORWARD_* macros generated?

https://searchfox.org/mozilla-central/rev/7abb9117c8500ed20833746c9f8e800fce3a4688/xpcom/idl-parser/xpidl/header.py#546 and https://searchfox.org/mozilla-central/rev/7abb9117c8500ed20833746c9f8e800fce3a4688/xpcom/idl-parser/xpidl/header.py#581

This is a reasonably commonly used feature in the tree (100+ uses in mozilla-central, certainly).

Ben, I'd really like to understand what's confusing about the MOZ_CAN_RUN_SCRIPT setup so I can improve the documentation. I'm generally available during work hours US/Eastern and can talk on IRC or video or whatever would work best for you if you'd like...

I need to read through the mozilla-can-run-scripts code and get a proper grasp of what it's doing.

I assume the documentation I linked above does not really answer your questions?

Also, where are the NS_FORWARD_* macros generated?

https://searchfox.org/mozilla-central/rev/7abb9117c8500ed20833746c9f8e800fce3a4688/xpcom/idl-parser/xpidl/header.py#546 and https://searchfox.org/mozilla-central/rev/7abb9117c8500ed20833746c9f8e800fce3a4688/xpcom/idl-parser/xpidl/header.py#581

This is a reasonably commonly used feature in the tree (100+ uses in mozilla-central, certainly).

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

Just to be clear, if the goal is just "get stuff compiling ASAP", then I don't understand all the work being done here.

Thanks for your support, Boris. Yes, the aim here is to get up an running again, then we improve the code base later not under bustage-fix pressure.

I followed your suggestion in TB's compose code, but the JS Account stuff with all the macros is just a mystery. Add to that, that I need a working local system to work on other stuff, and enabling ac_add_options --enable-clang-plugin in the mozconfig and disabling it again causes a recompile of about 30 minutes on one of the fastest machines available in the market. So IOW, it's not fun.

As far as I can see the compile error for JS Account happens on
2:12.90 In file included from c:/mozilla-source/comm-central/comm/mailnews/jsaccount/src/JaCompose.cpp:7
where the include file #include "JaCompose.h" is included. That in turn creates a lot of code in the "forward macros", and as far as I see, the code in those macros would need to be decorated with MOZ_CAN_RUN_SCRIPT_BOUNDARY. So personally, I cannot implement your suggestion.

This is what is needed to switch off JS Account. It's a pain in nsMailModule.cpp, so I added a #define to make it easier next time.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/ca1e6b213293
temporarily switch off JS Account. rs=bustage-fix
https://hg.mozilla.org/comm-central/rev/d916bdb4640d
Backed out changeset 063e8f6bc499 to re-enable the clang plugin. a=backout
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/dad748b418b7
Follow-up: fix typo in #define. rs=typo-fix DONTBUILD
Attached patch 1536047-Windows.patch (obsolete) — — Splinter Review

Geoff tells me that this would have worked if the added code had been lower in the file:

09:59:59 - darktrojan: jorgk, your change is too high up the file https://hg.mozilla.org/try-comm-central/rev/7bdc9d43573166dc4c553247a0cf3d3ca159eae1
10:00:37 - darktrojan: mozconfig.vs-latest calls mozconfig.clang-cl which sets ENABLE_CLANG_PLUGIN=1

We can consider that option if JS Account resists.

OK, so this should work according to Geoff, so let's push this to the beta.

Attachment #9051935 - Attachment is obsolete: true

@JörgK: What's the bug for re-enabling JS Account?

None, we'll re-enable it here after you've sent a patch to fix the compile errors ;-) - But seriously, if we can't get it fixed within a few days, I'll land the beta patch here which disables the analysis. I hear that there will be big bustage coming in bug 1534292 ("to break the world", see comment #42).

When you disable such a feature, could you please at least file a bug to fix it?

We're currently completely buried just to pay rent, so we won't be able to work on it anytime soon.

I don't know what you gain by filing a bug. I manage the code base very actively and I know that you exist and rely on JS Account. You didn't enter any agreement with the project to maintain it yourself. You also know that the functioning of JS Account is not guaranteed, at least not on Daily. As any other feature, it can go bust any minute. Now, please let me do my work and not waste time on comments like this one.

but the JS Account stuff with all the macros is just a mystery.

Why is it involved at all? If you put MOZ_CAN_RUN_SCRIPT_BOUNDARY as close as you can to the relevant callsites into Gecko that are now MOZ_CAN_RUN_SCRIPT, the JS Account stuff should not matter in any way, I would think. Or are there JS Account impls of core Gecko interfaces (like editor) that are now MOZ_CAN_RUN_SCRIPT? (I sort of doubt that.)

and enabling ac_add_options --enable-clang-plugin in the mozconfig and disabling it again causes a recompile of about 30 minutes

Uh... why are you doing this? Use two separate mozconfigs with two separate objdirs.

So personally, I cannot implement your suggestion.

I have zero context for this claim. Can you please attach your patch which is having this problem with JS Accounts or point to it if it's already attached to this bug? I really feel like you've somehow painted yourself into a corner with some changes that are not strictly needed to fix the immediate bustage but do require you to disable JS Accounts. For example, I would expect absolutely no .idl changes in a minimal patch that just adds the minimum required MOZ_CAN_RUN_SCRIPT_BOUNDARY annotations. Is that true about your patch?

I hear that there will be big bustage coming in bug 1534292

Just to reiterate:

  1. Timeframe there is unknown. I have no concrete plans to work on it in the next month or two, and no one else is planning to either.

  2. Disabling the analysis is the ostrich approach to security in this case. The right thing is to fix the issues the analysis uncovers. It may be good to start on that, slowly, before it becomes an emergency if someone does pick up bug 1534292.

bz, just FYI, JsAccount works by implementing the nsIMsg* TB APIs (nsIMsgFolder, incoming server, message etc.) in C++, and then letting C++ delegate/forward the call to a JS implementation. To add complexity, some of the interface functions remain implemented in C++, only some functions are implemented in JS. And, if I'm not mistaken (not 100% sure), JsAccount also exposes some C++ implementations to JS, so that it can use these as helper functions for important core functions that are implemented in C++. So, there's a quite complex interaction between JS and C++.

This just as background information. I have no idea what part of that is relevant here.

Hi Boris, thanks for your continued support. Please note that the TB project has 12 paid staff now. My roll is to keep the show on the road, fixing bustage, releases, regressions, sheriffing, etc. That said, I can't fix everything that breaks alone. I will mostly get it to the point where we can build, tests are mostly green, so others can work. If that means temporary fixes, switching off stuff or tests, so be it. I keep track of it. So I've handed that stuff over to BenC. If he doesn't fix it very quickly, I'll put JS Account back and disable the static analysis, that's working on beta now.

So for now, I won't be making any more attempts or attaching any patches.

What I said in comment #51:

As far as I can see the compile error for JS Account happens on
2:12.90 In file included from c:/mozilla-source/comm-central/comm/mailnews/jsaccount/src/JaCompose.cpp:7
where the include file #include "JaCompose.h" is included. That in turn creates a lot of code in the "forward macros", and as far as I see, the code those macros create would need to be decorated with MOZ_CAN_RUN_SCRIPT_BOUNDARY. So personally, I cannot implement your suggestion.

Yes, the preprocessor turns the "forward macros" into what you see in comment #24:

#define NS_FORWARD_NSIMSGCOMPOSE(_to) \
  NS_IMETHOD SetDocumentCharset(const char * charset) override { return _to SetDocumentCharset(charset); } \

and the _to parameter which is substituted by the macro invocation is what the analysis complains about.

So unless I understand it incorrectly, the NS_IMETHOD SetDocumentCharset would need to receive the MOZ_CAN_RUN_SCRIPT_BOUNDARY, and that's not possible since it's generated by the macro.

If you put MOZ_CAN_RUN_SCRIPT_BOUNDARY as close as you can to the relevant callsites
into Gecko that are now MOZ_CAN_RUN_SCRIPT, the JS Account stuff should not matter
in any way, I would think. Or are there JS Account impls of core Gecko interfaces
(like editor) that are now MOZ_CAN_RUN_SCRIPT? (I sort of doubt that.)

As I said, I don't see any call sites into Gecko-MOZ_CAN_RUN_SCRIPT functions other than the ones I fixed in our compose code. What appears to be happening is that JS Account somehow now forwards to compose interfaces that have been made MOZ_CAN_RUN_SCRIPT since they call such Gecko functions. So in a way, yes, JS Account appears to implement MOZ_CAN_RUN_SCRIPT functions, but not from Gecko, but from Mailnews.

Weren't attachment 9051818 [details] [diff] [review] and attachment 9051830 [details] [diff] [review], which both don't compile, attempts to deliver strong references?

No offence intended, but I won't touch it any more for now. I expect to wake up one morning and find this fixed by BenC who is working from NZ (I'm in Europe, we have 12 hours time difference between the two).

Ben, this one is for you. With two lines you can re-enable JS Account. I intend to leave the #if stuff in place for the next time we need to disable it.

The NS_IMETHOD SetDocumentCharset would need to receive the MOZ_CAN_RUN_SCRIPT_BOUNDARY

Why?

Weren't attachment 9051818 [details] [diff] [review] [diff] [review] and attachment 9051830 [details] [diff] [review] [diff] [review], which both don't compile, attempts to deliver strong references?

Yes, but those are all on top of https://bug1536047.bmoattachments.org/attachment.cgi?id=9051678 which is already wrong if you're aiming for a minimal fix, because it's changing .idl. If the issue was that editor->SelectAll() is MOZ_CAN_RUN_SCRIPT (which it is), then marking nsMsgCompose::ConvertAndLoadComposeWindow as MOZ_CAN_RUN_SCRIPT_BOUNDARY and the strong-ref change in https://bug1536047.bmoattachments.org/attachment.cgi?id=9051678 around the "@@ -924,17 +924,18 @@ nsMsgCompose::ConvertAndLoadComposeWindo" bits would have been enough. Similar for other bits where actual calls into editor are happening.

It's probably not going to help any more in this bug, but I am trying to make sure that future changes like this go more smoothly, so trying to make sure you understand exactly what my proposed way of dealing with them is, because I suspect it's a lot less disruptive and painful for everyone involved than what happened here....

Thanks, Boris, the penny has dropped now. I will revert most of the compose changes and only leave MOZ_CAN_RUN_SCRIPT_BOUNDARY on nsMsgCompose::ConvertAndLoadComposeWindow().

I'll file a new bug to put it all back and to fix JS Account.

Keywords: leave-open
Target Milestone: --- → Thunderbird 68.0

OK, that reverts most of https://hg.mozilla.org/comm-central/rev/58d5b2195fc8.

Attachment #9051818 - Attachment is obsolete: true
Attachment #9051830 - Attachment is obsolete: true

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/e5cad39148cd
Re-enable JS Account. r=me
https://hg.mozilla.org/comm-central/rev/b1b1782857a7
Revert rev 58d5b2195fc8 and only use MOZ_CAN_RUN_SCRIPT_BOUNDARY once in nsMsgCompose.cpp. r=me

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Blocks: 1536535
Comment on attachment 9051678 [details] [diff] [review]
1536047-MOZ_CAN_RUN_SCRIPT.patch - WIP

Nothing to review here, it mostly got backed out apart from the changes in `nsMsgCompose::ConvertAndLoadComposeWindow()`.
Attachment #9051678 - Flags: review?(benc)

Re-enable JS Account. r=me

Thank you

Comment on attachment 9052034 [details] [diff] [review]
1536047-re-enable-js-account.patch

> Ben, this one is for you.

r+
Attachment #9052034 - Flags: review+

Oh, I meant BenC, to use when fixing the C++ bustage. But we have bug 1536535 for that now.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: