1497513 - Want to make some filter methods scriptable

Status: RESOLVED FIXED

(MailNews Core :: Filters, enhancement)

Description

[[e-mail]]

I'd liked to call some filter methods but they've been marked as [noscript] because of the non-standard string passing convention that they use.

Fortunately the problem only prevents passing strings from C++ to JS but passing strings from JS to C++ is not affected so this isn't necessary.

Comment 1

[[e-mail]]

Attachment: Proposed patch

Comment 2

[:aceman]

Comment on attachment 9015527 [details] [diff] [review]
Proposed patch

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

I'm not sure about this stuff, Magnus could know better.

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9015527 [details] [diff] [review]
Proposed patch

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

But that would be a problem when we move filtering implementations to JavaScript, right?
Looks to me it would be preferable to just do away with the null-separated list of headers, and use something more standard like comma separated headers.

Comment 4

[Jorg K (CEST = GMT+2)]

Or an array? M-C offer arrays in IDL these days, see an example here:
https://hg.mozilla.org/mozilla-central/rev/a33fb1d3cda0#l3.19

Comment 5

[:aceman]

Good idea.
Commas may also be problematic as they could occur in the data.
Maybe we could activate the '[array, size_is(headerSize)] in string headers,' arguments (make the headers an array), which somebody seems to have already thought about in the past.

Neil, would you be able to do that (remove the single string argument and make it an array) ? And would it then be usable from JS?

Comment 6

[:aceman]

It is seen in the patch itself:
-    // Marking noscript because "headers" is actually a null-separated
-    // list of headers, which is not scriptable.
-    [noscript] void MatchHdr(in nsIMsgDBHdr msgHdr, in nsIMsgFolder folder,
+    void MatchHdr(in nsIMsgDBHdr msgHdr, in nsIMsgFolder folder,
                   in nsIMsgDatabase db,
                   in string headers,
->                 //                  [array, size_is(headerSize)] in string headers,
                   in unsigned long headerSize, out boolean result);

Comment 7

[Magnus Melin [:mkmelin]]

Yeah comma separation would not work if it's the complete header. (I read it as the header name, not the complete header line).

Comment 8

[:aceman]

The MatchHdr() calls into other functions 10 levels deep (which all would need a change) passing this 'headers' arguments.
I'm also not sure whether it only contains header names or values too (or the full msgs header lines), as e.g. in https://dxr.mozilla.org/comm-central/source/comm/mailnews/base/search/src/nsMsgBodyHandler.cpp#146 it still does a lot of parsing of the m_headers string.

Comment 9

[Ben Bucksch (:BenB)]

Given what acemen said (comment 8), can we land the patch as-is, and make a followup bug to make this nicer with arrays?
From what I understand, Neil's patch is a direct improvement, i.e. doesn't regress anything that already works, and makes new things work that didn't work before.

Comment 10

[Magnus Melin [:mkmelin]]

Can someone verify if it's the header names or values? If it's just the names it's simple enough to comma separate and avoid the larger changes.

I would imagine it is the whole headers though, and that's why null separation was used. 

I don't think we want to make a JavaScript implementation non-workable, especially given that filtering is an excellent candidate for a js rewrite.

Comment 11

[[e-mail]]

Attachment: Use ACString

... you can't argue that you can't have nulls in ACStrings, because you can.

Comment 12

[Ben Bucksch (:BenB)]

Comment on attachment 9016715 [details] [diff] [review]
Use ACString

I think ACString is a good idea.
I've browsed over the patch and it looks sane to me.
I think this is an improvement over status quo.

Comment 13

[Ben Bucksch (:BenB)]

@Neil: Could you please state here what you did to manually test this latest ACString patch and to ensure that it doesn't break anything?
Could you please, in addition to manual tests, also push a try run?

Comment 14

[Jorg K (CEST = GMT+2)]

Try run is green:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=78219475a2aacc33c6b68865620f46bbe261ff21

Sadly we had an influx of Linux MozMill bustage and telemetry issues, but we're on the way to addressing those.

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: 1497513.diff - Patch for landing

Not the ideal solution, but it will do for now. I have no idea why Substring was used at three call sites, unless I'm missing something it should be nsDependentCString in all three cases.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8aebb2ed9307
make some filter methods scriptable, replace string with ACString for 'headers' parameter. r=jorgk

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: 1497513-follow-up.patch

I did mess this up :-( - crashes in debug mode with a message that nsDependentCSubstring() should be used. Sorry.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6dda9a7770fc
fix incorrect use of nsDependentCString. r=me

Comment 19

[Ben Bucksch (:BenB)]

Thanks, Jörg!

Comment 20

[[e-mail]]

(In reply to Jorg K from comment #14)
> Try run is green:
> https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=78219475a2aacc33c6b68865620f46bbe261ff21

Ah, you found it ;-) I would have posted it but I had a stomach ache.

(In reply to Jorg K from comment #15)
> Not the ideal solution, but it will do for now. I have no idea why Substring
> was used at three call sites, unless I'm missing something it should be
> nsDependentCString in all three cases.

Those three C++ callers don't pass null-terminated strings (well, the last character of the string is null, but that's not what nsDependentCString is looking for). Substring is slightly less typing and slightly more type-safe than nsDependent(C)Substring.

Comment 21

[Jorg K (CEST = GMT+2)]

You'll handle the uplift request here, right?

Well, initially I wanted to use nsDependentCString to avoid making copies of strings. I still think nsDependentCSubstring is the better solution here, since Substring(char*, ...) would have promoted the raw string to a "smart" string first, no?

Comment 22

[Ben Bucksch (:BenB)]

Neil says that both are equivalent after compile, but Substring() is type-safe during compile/development, so strictly better.
But for the same reason, it doesn't make much of a difference. We can leave it as-is. Just for the next time.

Comment 23

[Ben Bucksch (:BenB)]

Comment on attachment 9017707 [details] [diff] [review]
1497513.diff - Patch for landing

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: * Filtering will not work in Owl.

• This change will not get beta testing and bugs might not be found

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce:

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Plays with C strings that come from senders, so you be the judge :)

String changes made/needed:

Comment 24

[Ben Bucksch (:BenB)]

Comment on attachment 9017707 [details] [diff] [review]
1497513.diff - Patch for landing

Sorry, I keep picking the wrong flags. There are too many of them :)

Comment 25

[Jorg K (CEST = GMT+2)]

Comment on attachment 9017707 [details] [diff] [review]
1497513.diff - Patch for landing

Sorry, this landed on TB 64, so it is already in the current beta. Did you mean ESR?

Comment 26

[Ben Bucksch (:BenB)]

Comment on attachment 9017707 [details] [diff] [review]
1497513.diff - Patch for landing

Sorry, I didn't know that this is in beta. Can you maybe mark it approval-comm-beta+ , so that we know it landed?

[Approval Request Comment]
User impact if declined:

• Filtering in Owl will not work.
• Apparently no other user visible effect

Testing completed (on c-c, etc.):
Has been in TB 64 beta and 65 beta for a while.

Risk to taking this patch (and alternatives if risky):
Messes with C strings of set by message senders, so you be the judge :)

Comment 27

[Jorg K (CEST = GMT+2)]

Comment on attachment 9017707 [details] [diff] [review]
1497513.diff - Patch for landing

Good for 60.5.

Comment 28

[Jorg K (CEST = GMT+2)]

TB 60.5.0 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/efaa157963c853da0e396051b058bc6a1aaa454f
https://hg.mozilla.org/releases/comm-esr60/rev/f551d631100b38281cbb8dc0584e2e3a0e7fb054