1962454 - Pass origin instead of host/port to Windows notification

Status: RESOLVED FIXED

(Toolkit :: Alerts Service, task)

Comment 1

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1962454 - Pass origin instead of host/port to Windows notification r=nalexander,nrishel

This is to match NotificationDB that uses origin.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5cb7eed6c1d2
Pass origin instead of host/port to Windows notification r=nalexander,win-reviewers,firefox-desktop-core-reviewers ,gstoll

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a3539d67b3cc
Revert "Bug 1962454 - Pass origin instead of host/port to Windows notification r=nalexander,win-reviewers,firefox-desktop-core-reviewers ,gstoll" for causing xpc failures @test_backgroundtask_experiments.js.

Comment 4

[agoloman]

Backed out for causing xpc failures @test_backgroundtask_experiments.js.

• Backout link
• Push with failures
• Failure Log

Comment 5

[Kagami Rosylight [:saschanaz] (they/them)]

Somehow the CI log doesn't have details, but local run shows it:

toolkit/components/backgroundtasks/tests/xpcshell/test_backgroundtask_experiments.js
  FAIL toolkit/components/backgroundtasks/tests/xpcshell/test_backgroundtask_experiments.js - xpcshell return code: 0
ERROR Unexpected exception TypeError: can't access property "args", infoMap.showAlert is undefined at D:/gecko/obj-aarch64-pc-windows-msvc/_tests/xpcshell/toolkit/components/backgroundtasks/tests/xpcshell/test_backgroundtask_experiments.js:189
test_backgroundtask_caps@D:/gecko/obj-aarch64-pc-windows-msvc/_tests/xpcshell/toolkit/components/backgroundtasks/tests/xpcshell/test_backgroundtask_experiments.js:189:15
async*_run_next_test/<@d:\gecko\testing\xpcshell\head.js:1759:22
_run_next_test@d:\gecko\testing\xpcshell\head.js:1759:38
run@d:\gecko\testing\xpcshell\head.js:808:9
_do_main@d:\gecko\testing\xpcshell\head.js:245:6
_execute_test@d:\gecko\testing\xpcshell\head.js:596:5
@-e:1:1

With some debugging I learned that the message background task is somehow accessing the new attribute .origin (which throws error when with system principal). But I'm not sure where and why exactly the access happens, as even with artificial MOZ_CRASH I can't really see the crash stack. Any idea, Nick?

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Kagami Rosylight [:saschanaz] (they/them) from comment #5)

Somehow the CI log doesn't have details, but local run shows it:

toolkit/components/backgroundtasks/tests/xpcshell/test_backgroundtask_experiments.js
  FAIL toolkit/components/backgroundtasks/tests/xpcshell/test_backgroundtask_experiments.js - xpcshell return code: 0
ERROR Unexpected exception TypeError: can't access property "args", infoMap.showAlert is undefined at D:/gecko/obj-aarch64-pc-windows-msvc/_tests/xpcshell/toolkit/components/backgroundtasks/tests/xpcshell/test_backgroundtask_experiments.js:189
test_backgroundtask_caps@D:/gecko/obj-aarch64-pc-windows-msvc/_tests/xpcshell/toolkit/components/backgroundtasks/tests/xpcshell/test_backgroundtask_experiments.js:189:15
async*_run_next_test/<@d:\gecko\testing\xpcshell\head.js:1759:22
_run_next_test@d:\gecko\testing\xpcshell\head.js:1759:38
run@d:\gecko\testing\xpcshell\head.js:808:9
_do_main@d:\gecko\testing\xpcshell\head.js:245:6
_execute_test@d:\gecko\testing\xpcshell\head.js:596:5
@-e:1:1

With some debugging I learned that the message background task is somehow accessing the new attribute .origin (which throws error when with system principal). But I'm not sure where and why exactly the access happens, as even with artificial MOZ_CRASH I can't really see the crash stack. Any idea, Nick?

Sure. What's happening is that the message task mocks the alert mechanism, dumping to stdout instead; the failing test runs the task and parses that output. Around https://searchfox.org/mozilla-central/rev/25a38ac3b851b63e7d85a850940c354609e8126d/toolkit/components/backgroundtasks/BackgroundTask_message.sys.mjs#140-145, we're trying to serialize the alert datum naively, and making the getter fail for specific principals breaks the existing contract (which is that you can serialize the datum). You could strip the origin or otherwise work around it and I expect everything would work just fine. See how that works for you?

Comment 7

[Kagami Rosylight [:saschanaz] (they/them)]

For now I made it never throw because I don't see an easy way to make JSON.stringify work without convoluting the implementation. I could make it return null but turns out it's not a thing. Bug 1966371.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/88f4bef6432c
Pass origin instead of host/port to Windows notification r=nalexander,win-reviewers,firefox-desktop-core-reviewers ,gstoll

Comment 9

[agoloman]

https://hg.mozilla.org/mozilla-central/rev/88f4bef6432c

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This relanded with me as patch author. Can you back out and reland with you as author?

Comment 11

[Kagami Rosylight [:saschanaz] (they/them)]

First I'm not sure that's worth, a landed patch is more important to me than whose name is on it.
Second I'm not sherrif and can't backout?

Comment 12

[Kagami Rosylight [:saschanaz] (they/them)]

What happened btw? 🤔

Comment 13

[Kagami Rosylight [:saschanaz] (they/them)]

3. This was already backed out once and still both landings used your name (Aryx), so nothing makes sure that the next landing will properly use my name. I pinged the right people and filed a bug.

Comment 14

[Kagami Rosylight [:saschanaz] (they/them)]

Okay, turns out I already had you as the author locally. The detailed log is gone as the initial patch was created months ago, but I'll request backout to make the blame less confusing.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2d57a5c5780
Revert "Bug 1962454 - Pass origin instead of host/port to Windows notification r=nalexander,win-reviewers,firefox-desktop-core-reviewers ,gstoll" as requested by krosylight.

Comment 16

[Serban Stanca [:SerbanS]]

Backed out as requested by krosylight.

• Backout link

Comment 17

[Kagami Rosylight [:saschanaz] (they/them)]

Thanks! Now let's hope the new landing goes with the right author.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/721132c7e287
Pass origin instead of host/port to Windows notification r=nalexander,win-reviewers,firefox-desktop-core-reviewers ,gstoll

Comment 19

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/721132c7e287