Want to make some filter methods scriptable

RESOLVED FIXED in Thunderbird 64.0

Status

enhancement
RESOLVED FIXED
10 months ago
6 months ago

People

(Reporter: neil, Assigned: neil)

Tracking

unspecified
Thunderbird 64.0

Thunderbird Tracking Flags

(thunderbird_esr6065+ fixed, thunderbird64 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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.
Posted patch Proposed patch (obsolete) — Splinter Review
Assignee: nobody → neil
Attachment #9015527 - Flags: review?(acelists)
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.
Attachment #9015527 - Flags: review?(acelists) → review?(mkmelin+mozilla)
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.
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
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?
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);
Yeah comma separation would not work if it's the complete header. (I read it as the header name, not the complete header line).
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.
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.
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.
Attachment #9015527 - Flags: review?(mkmelin+mozilla)
Posted patch Use ACString (obsolete) — Splinter Review
... you can't argue that you can't have nulls in ACStrings, because you can.
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.
Attachment #9016715 - Flags: review?(acelists)
@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?
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.
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.
Attachment #9015527 - Attachment is obsolete: true
Attachment #9016715 - Attachment is obsolete: true
Attachment #9016715 - Flags: review?(acelists)
Attachment #9017707 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8aebb2ed9307
make some filter methods scriptable, replace string with ACString for 'headers' parameter. r=jorgk
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
I did mess this up :-( - crashes in debug mode with a message that nsDependentCSubstring() should be used. Sorry.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6dda9a7770fc
fix incorrect use of nsDependentCString. r=me
Thanks, Jörg!
(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.
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?
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 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:

Attachment #9017707 - Flags: approval-mozilla-beta?

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 :)

Attachment #9017707 - Flags: approval-mozilla-beta? → approval-comm-beta?
Attachment #9017786 - Flags: approval-comm-beta?

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?

Attachment #9017707 - Flags: approval-comm-beta?
Attachment #9017786 - Flags: approval-comm-beta?

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 :)

Attachment #9017707 - Flags: approval-comm-esr60?
Attachment #9017786 - Flags: approval-comm-esr60?

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

Good for 60.5.

Attachment #9017707 - Flags: approval-comm-esr60? → approval-comm-esr60+
Attachment #9017786 - Flags: approval-comm-esr60? → approval-comm-esr60+
You need to log in before you can comment on or make changes to this bug.