Closed Bug 1900657 Opened 1 year ago Closed 1 month ago

Intermittent comm/mail/components/extensions/test/browser/browser_ext_quickFilter.js | Should set text filter - null == ""

Categories

(Thunderbird :: Add-Ons: Extensions API, defect, P5)

Product:
Thunderbird
Component:
Add-Ons: Extensions API
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
148 Branch

People

(Reporter: intermittent-bug-filer, Assigned: john)

References

Details

(Keywords: intermittent-failure, intermittent-testcase)

Attachments

(1 file, 2 obsolete files)

Bug 1900657 - Correctly use null to clear the quickFilter in the mailTabs API. r=mkmelin
48 bytes, text/x-phabricator-request
Details | Review
Summary: Intermittent comm/mail/components/extensions/test/browser/browser_ext_quickFilter.js | single tracking bug → Intermittent comm/mail/components/extensions/test/browser/browser_ext_quickFilter.js | Should set text filter - null == ""

This seems not so intermittent with TSAN build and test.
See https://treeherder.mozilla.org/jobs?repo=try-comm-central&selectedTaskRun=KtcwhpArSfyirfVokd9U8w.0&revision=a12baab185ee31c44daf3aa04fe815dd9789eace
Specifically the repetition of the error in bct9.

https://treeherder.mozilla.org/logviewer?job_id=500909314&repo=try-comm-central&lineNumber=2126

I find the line that lists undefined value in the log rather disturbing.
Obivously, the |flagged| has NOT been set a value, or is it OK that it stays |undefined|?

[task 2025-03-25T17:17:20.894Z] 17:17:20     INFO - Extension loaded
[task 2025-03-25T17:17:20.895Z] 17:17:20     INFO - Buffered messages logged at 17:17:20
[task 2025-03-25T17:17:20.896Z] 17:17:20     INFO - TEST-PASS | comm/mail/components/extensions/test/browser/browser_ext_quickFilter.js | show should be propagated to the filterer - true == true - 
[task 2025-03-25T17:17:20.896Z] 17:17:20     INFO - TEST-PASS | comm/mail/components/extensions/test/browser/browser_ext_quickFilter.js | Quick filter bar should be visible when the filter requests it be shown - true == true - 
[task 2025-03-25T17:17:20.897Z] 17:17:20     INFO - TEST-PASS | comm/mail/components/extensions/test/browser/browser_ext_quickFilter.js | Should set text filter - "test" == "test" - 
[task 2025-03-25T17:17:20.897Z] 17:17:20     INFO - TEST-PASS | comm/mail/components/extensions/test/browser/browser_ext_quickFilter.js | Should update the search bar input - "test" == "test" - 
[task 2025-03-25T17:17:20.897Z] 17:17:20     INFO - TEST-PASS | comm/mail/components/extensions/test/browser/browser_ext_quickFilter.js | Should set the subject filter - true == true - 
[task 2025-03-25T17:17:20.897Z] 17:17:20     INFO - TEST-PASS | comm/mail/components/extensions/test/browser/browser_ext_quickFilter.js | Should reflect toggle state in UI - true == true - 
[task 2025-03-25T17:17:20.897Z] 17:17:20     INFO - TEST-PASS | comm/mail/components/extensions/test/browser/browser_ext_quickFilter.js | Should update flagged state - "undefined" == "undefined" - 
[task 2025-03-25T17:17:20.898Z] 17:17:20     INFO - TEST-PASS | comm/mail/components/extensions/test/browser/browser_ext_quickFilter.js | Should reflect flagged state - false == false - 
[task 2025-03-25T17:17:20.899Z] 17:17:20     INFO - TEST-PASS | comm/mail/components/extensions/test/browser/browser_ext_quickFilter.js | show should be propagated to the filterer - true == true - 
[task 2025-03-25T17:17:20.899Z] 17:17:20     INFO - TEST-PASS | comm/mail/components/extensions/test/browser/browser_ext_quickFilter.js | Quick filter bar should be visible when the filter requests it be shown - true == true - 
[task 2025-03-25T17:17:20.900Z] 17:17:20     INFO - Buffered messages finished
[task 2025-03-25T17:17:20.901Z] 17:17:20     INFO - TEST-UNEXPECTED-FAIL | comm/mail/components/extensions/test/browser/browser_ext_quickFilter.js | Should set text filter - null == "" -

OTOH, it might be that the test needs to be rewritten:
https://searchfox.org/comm-central/source/mail/components/extensions/test/browser/browser_ext_quickFilter.js#228

    if (state.show) {
      Assert.equal(
        qfb.filterer.getFilterValue("text", true).text,
        state.text.text,  <--- *** This probably should be rewritten as state.text.text || ""    just like in the next test.
        "Should set text filter"
      );
      Assert.equal(
        gDefaultAbout3Pane.document
          .getElementById("qfb-qs-textbox")
          .shadowRoot.querySelector("input").value,
        state.text.text || "",    <==============================  Here is the pattern I mentioned above.
        "Should update the search bar input"
      );

(In reply to ISHIKAWA, Chiaki (Out of town, will return on Sunday) from comment #28)
...

OTOH, it might be that the test needs to be rewritten:
https://searchfox.org/comm-central/source/mail/components/extensions/test/browser/browser_ext_quickFilter.js#228

    if (state.show) {
      Assert.equal(
        qfb.filterer.getFilterValue("text", true).text,
        state.text.text,  <--- *** This probably should be rewritten as state.text.text || ""    just like in the next test.

Actually, maybe the line above the suggested line needs to be rewritten as follows. Given the assert output from the test in the log.

    qfb.filterer.getFilterValue("text", true).text || ""

                                                                    ^^^^^^

Whether having |gfb.filter.getFilterValue("text", true).text| null value is OK or not is another matter.

Assignee: nobody → ishikawa
Status: NEW → ASSIGNED
Component: Filters → Add-Ons: Extensions API

This is a timing issue. The attached patch is adding logging and it shows that clearing the text via

await browser.mailTabs.setQuickFilter({ text: { text: "" } });

sets the value to "", and calling

about3Pane.quickFilterBar.filterer.getFilterValue("text", true));

returns "", but after some time, the same command returns null.

I do not see where that happens. Should we enforce null when setting "" in the API, or spend more time on tracking this down?

Sounds like we should figure out why that's happening.

It happens here: https://searchfox.org/comm-central/rev/2d4928acbaee09dd983f01a2daaca64a3bac2367/mail/base/content/quickFilterBar.js#265-275

The preValue has text: "", and once

          const [postValue, update] = filterDef.onCommand(
            preValue,
            domNode,
            event,
            document
          );

is called, preValue is updated in-place to text: null and postValue is set to that as well. This is then used on this.filterer.setFilterValue().

I could investigate this further, but since initial cleared state is connected to text: null also in other places 1, my proposal here would be to no longer set text to "" but to null, as this aligns with what other code is using. This would change the expected test value, but since we are not exposing this to extension, I am fine with that.

Assignee: ishikawa → john
Blocks: tb-maintenance-2025-q4

I updated the Debug patch to inject a delay and wait for the late update of the filter value, including some additional logs to show what happens:

../mach test mail/components/extensions/test/browser/browser_ext_quickFilter.js 

 0:06.17 GECKO(2490442) console.log: "Set Filter Value" "unread" null
 0:06.17 GECKO(2490442) console.log: "Set Filter Value" "starred" null
 0:06.17 GECKO(2490442) console.log: "Set Filter Value" "addrBook" null
 0:06.17 GECKO(2490442) console.log: "Set Filter Value" "attachment" null
 0:06.19 GECKO(2490442) console.log: "Set Filter Value" "results" null
 0:06.19 GECKO(2490442) console.log: "Set Filter Value" "text" ({states:{recipients:false, sender:false, subject:false, body:false}, text:""})
 0:06.19 GECKO(2490442) console.log: "Set Filter Value" "results" null
 0:06.19 GECKO(2490442) console.log: WebExtensions: mailTabsApi:setQuickFilter:updatedValues:0ms {"states":{"recipients":false,"sender":false,"subject":false,"body":false},"text":""}
 0:06.31 GECKO(2490442) console.log: WebExtensions: mailTabsApi:setQuickFilter:updatedValues:125ms {"states":{"recipients":false,"sender":false,"subject":false,"body":false},"text":""}
 0:06.42 GECKO(2490442) console.log: "quickFilterBar:handlerDomId:preValue" ({states:{recipients:false, sender:false, subject:false, body:false}, text:""})
 0:06.43 GECKO(2490442) console.log: "quickFilterBar:handlerDomId:postValue" ({states:{recipients:false, sender:false, subject:false, body:false}, text:null})
 0:06.43 GECKO(2490442) console.log: "Set Filter Value" "text" ({states:{recipients:false, sender:false, subject:false, body:false}, text:null})
 0:06.45 GECKO(2490442) console.log: WebExtensions: mailTabsApi:setQuickFilter:updatedValues:250ms {"states":{"recipients":false,"sender":false,"subject":false,"body":false},"text":null}
 0:06.58 GECKO(2490442) console.log: WebExtensions: mailTabsApi:setQuickFilter:updatedValues:375ms {"states":{"recipients":false,"sender":false,"subject":false,"body":false},"text":null}
 0:06.70 GECKO(2490442) console.log: WebExtensions: mailTabsApi:setQuickFilter:updatedValues:500ms {"states":{"recipients":false,"sender":false,"subject":false,"body":false},"text":null}
 0:06.83 GECKO(2490442) console.log: WebExtensions: mailTabsApi:setQuickFilter:updatedValues:625ms {"states":{"recipients":false,"sender":false,"subject":false,"body":false},"text":null}
 0:06.96 GECKO(2490442) console.log: WebExtensions: mailTabsApi:setQuickFilter:updatedValues:750ms {"states":{"recipients":false,"sender":false,"subject":false,"body":false},"text":null}

I will now move on and fix the mailTabs API to correctly use null to clear the filter text.

As outlined in https://bugzilla.mozilla.org/show_bug.cgi?id=1900657#c60 (and the following),
the test fail is a real glitch caused by a delayed update of the text value to null by the
handlerDomId callback used by the autocomplete event.

This patch fixes the test fail by correctly clearing the filter text by setting it to null,
and waiting for browser.mailTabs.setQuickFilter() to resolve before validating the UI.

Attachment #9532722 - Attachment is obsolete: true
Attachment #9474997 - Attachment is obsolete: true
Keywords: checkin-needed-tb
Target Milestone: --- → 148 Branch

Pushed by john@thunderbird.net:
https://hg.mozilla.org/comm-central/rev/671cf534d377
Correctly use null to clear the quickFilter in the mailTabs API. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 1 month ago
Keywords: checkin-needed-tb
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: