Closed Bug 1875257 Opened 8 months ago Closed 8 months ago

Support proper set-permission for all permissions

Categories

(Remote Protocol :: Marionette, enhancement, P2)

Product:
Remote Protocol
Component:
Marionette
Type:
enhancement

Tracking

(firefox124 fixed)

Status:
RESOLVED FIXED
Milestone:
124 Branch
Tracking Flags:
Tracking Status
firefox124 --- fixed

People

(Reporter: saschanaz, Assigned: saschanaz)

References

Details

(Whiteboard: [webdriver:m10][webdriver:external])

Attachments

(2 files, 1 obsolete file)

Bug 1875257 - Part 1: Generalize permission handling of storage-access to all permissions r=whimboo
48 bytes, text/x-phabricator-request
Details | Review
Bug 1875257 - Part 3: Remove dom.screenwakelock.testing r=#dom-core
48 bytes, text/x-phabricator-request
Details | Review
No description provided.
Blocks: 1875065
Status: NEW → ASSIGNED
Attachment #9373473 - Attachment description: Bug 1875257 - Generalize permission handling of storage-access to all permissions r=whimboo → Bug 1875257 - Part 1: Generalize permission handling of storage-access to all permissions r=whimboo

Fixing one test to set permission properly and another to not trigger external network request.

Depends on D198836

test_driver.set_permission() should now just work with the marionette pref.

Depends on D198957

See Also: → 1875837
Pushed by krosylight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fd47336dc88d Part 1: Generalize permission handling of storage-access to all permissions r=whimboo,webdriver-reviewers https://hg.mozilla.org/integration/autoland/rev/adca043ae362 Part 2: Adjust notification tests r=asuth,dom-worker-reviewers https://hg.mozilla.org/integration/autoland/rev/0fff6ecf1c10 Part 3: Remove dom.screenwakelock.testing r=dom-core,edgar
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/44245 for changes under testing/web-platform/tests

Backed out for causing Android PermissionManager related wpt crashes.

  • Backout link
  • Push with failures
  • Failure Log
  • Failure line: PROCESS-CRASH | MOZ_ASSERT(PermissionAvailable(prin, aType)) [@ mozilla::PermissionManager::CommonTestPermissionInternal] | /notifications/permission.html
Flags: needinfo?(krosylight)
Upstream PR was closed without merging

The build failure bug 1877157 is blocking me from investigating further. That's an Android debug build specific crash and never happens elsewhere. 🤔

Flags: needinfo?(krosylight)

Hi Tim, can you help me investigating the crash in comment #6? This patch does not touch permission manager but only adding calls to nsIPermissionManager with add/removeFromPrincipal, which somehow makes permission not available early enough per the crash stack:

[task 2024-01-27T06:57:03.855Z] 06:57:03     INFO -  0  libxul.so!mozilla::PermissionManager::CommonTestPermissionInternal(nsIPrincipal*, nsIURI*, mozilla::OriginAttributes const*, int, nsTSubstring<char> const&, unsigned int*, bool, bool) [PermissionManager.cpp:1e79d9e65ec0577bb7d698c86edc6f35b4f92ce8 : 2446 + 0x0]
[task 2024-01-27T06:57:03.855Z] 06:57:03     INFO -      rax = 0x000079d95ee0f1f8    rdx = 0x0000000000000001
[task 2024-01-27T06:57:03.855Z] 06:57:03     INFO -      rcx = 0x000079d9831ffc20    rbx = 0x000079d96b285d60
[task 2024-01-27T06:57:03.855Z] 06:57:03     INFO -      rsi = 0x000079d96c338bf3    rdi = 0x000079d9832015b4
[task 2024-01-27T06:57:03.855Z] 06:57:03     INFO -      rbp = 0x000079d96b285e10    rsp = 0x000079d96b285d40
[task 2024-01-27T06:57:03.855Z] 06:57:03     INFO -       r8 = 0x0000000000004335     r9 = 0x000079d96b288450
[task 2024-01-27T06:57:03.855Z] 06:57:03     INFO -      r10 = 0x000079d96b285640    r11 = 0x0000000000000246
[task 2024-01-27T06:57:03.855Z] 06:57:03     INFO -      r12 = 0x0000000000000000    r13 = 0x000079d9574d4900
[task 2024-01-27T06:57:03.855Z] 06:57:03     INFO -      r14 = 0x0000000000000000    r15 = 0x000079d958139200
[task 2024-01-27T06:57:03.855Z] 06:57:03     INFO -      rip = 0x000079d96322f42c
[task 2024-01-27T06:57:03.855Z] 06:57:03     INFO -     Found by: given as instruction pointer in context
[task 2024-01-27T06:57:03.855Z] 06:57:03     INFO -  1  libxul.so!mozilla::PermissionManager::CommonTestPermission(nsIPrincipal*, int, nsTSubstring<char> const&, unsigned int*, unsigned int, bool, bool, bool) [PermissionManager.cpp:1e79d9e65ec0577bb7d698c86edc6f35b4f92ce8 : 3883 + 0x1c]
[task 2024-01-27T06:57:03.855Z] 06:57:03     INFO -      rbx = 0x0000000000000000    rbp = 0x000079d96b285e90
[task 2024-01-27T06:57:03.855Z] 06:57:03     INFO -      rsp = 0x000079d96b285e20    r12 = 0x000079d958139200
[task 2024-01-27T06:57:03.855Z] 06:57:03     INFO -      r13 = 0x000079d96b285e58    r14 = 0x0000000000000001
[task 2024-01-27T06:57:03.855Z] 06:57:03     INFO -      r15 = 0x000079d9574d4900    rip = 0x000079d96322e96a
[task 2024-01-27T06:57:03.855Z] 06:57:03     INFO -     Found by: call frame info
[task 2024-01-27T06:57:03.855Z] 06:57:03     INFO -  2  libxul.so!mozilla::PermissionManager::TestPermissionFromPrincipal(nsIPrincipal*, nsTSubstring<char> const&, unsigned int*) [PermissionManager.cpp:1e79d9e65ec0577bb7d698c86edc6f35b4f92ce8 : 2364 + 0x1b]
[task 2024-01-27T06:57:03.855Z] 06:57:03     INFO -      rbx = 0x000079d96b285ef0    rbp = 0x000079d96b285ec0
[task 2024-01-27T06:57:03.855Z] 06:57:03     INFO -      rsp = 0x000079d96b285ea0    r12 = 0x0000000000000002
[task 2024-01-27T06:57:03.855Z] 06:57:03     INFO -      r13 = 0x000079d9574ce4b8    r14 = 0x000079d9574ce490
[task 2024-01-27T06:57:03.855Z] 06:57:03     INFO -      r15 = 0x0000000000000030    rip = 0x000079d96322e9f8
[task 2024-01-27T06:57:03.855Z] 06:57:03     INFO -     Found by: call frame info
[task 2024-01-27T06:57:03.855Z] 06:57:03     INFO -  3  libxul.so!mozilla::PermissionDelegateHandler::PopulateAllDelegatedPermissions() [PermissionDelegateHandler.cpp:1e79d9e65ec0577bb7d698c86edc6f35b4f92ce8 : 295 + 0x13]
[task 2024-01-27T06:57:03.855Z] 06:57:03     INFO -      rbx = 0x000079d96b285ef0    rbp = 0x000079d96b286060
[task 2024-01-27T06:57:03.855Z] 06:57:03     INFO -      rsp = 0x000079d96b285ed0    r12 = 0x0000000000000002
[task 2024-01-27T06:57:03.855Z] 06:57:03     INFO -      r13 = 0x000079d9574ce4b8    r14 = 0x000079d9574ce490
[task 2024-01-27T06:57:03.856Z] 06:57:03     INFO -      r15 = 0x0000000000000030    rip = 0x000079d96322405f
[task 2024-01-27T06:57:03.856Z] 06:57:03     INFO -     Found by: call frame info
[task 2024-01-27T06:57:03.856Z] 06:57:03     INFO -  4  libxul.so!nsGlobalWindowInner::InitDocumentDependentState(JSContext*) [nsGlobalWindowInner.cpp:1e79d9e65ec0577bb7d698c86edc6f35b4f92ce8 : 1814 + 0xc]
[task 2024-01-27T06:57:03.856Z] 06:57:03     INFO -      rbx = 0x000079d955a18000    rbp = 0x000079d96b2860b0
[task 2024-01-27T06:57:03.856Z] 06:57:03     INFO -      rsp = 0x000079d96b286070    r12 = 0x000079d96b286070
[task 2024-01-27T06:57:03.856Z] 06:57:03     INFO -      r13 = 0x000079d96b286070    r14 = 0x000079d955a18040
[task 2024-01-27T06:57:03.856Z] 06:57:03     INFO -      r15 = 0x000079d955a180e8    rip = 0x000079d9637fe5d4
[task 2024-01-27T06:57:03.856Z] 06:57:03     INFO -     Found by: call frame info

But I'm unsure why the permission would ever be unavailable, can you give me some hint?

Flags: needinfo?(tihuang)

Sorry for the late response.

The permissions for the content process are sent during launching the content process and initiating the window global parent. So, potentially, we will hit the crash if we access permissions anytime before permissions are available in content processes. But your change doesn't seem to introduce permission checks before these two initiations.

Also, the crash is caused by checking permission for permission delegation when initiating the top-level window in the content process. The permission should be available by then unless your patch changes the timing there.

Flags: needinfo?(tihuang)

I tried investigating but didn't have luck. I assume that async tasks triggered by each notification constructor would affect something after test run, but preventing task by disabling the permission did not fix the crash. For now I'll modify the test to wait for onshow event, but given this is unsupported on Android, it'll be TIMEOUT/NOTRUN there, which is anyway consistent to other existing test results. We'll probably have to revisit this when implementing onshow support.

Pushed by krosylight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4b036bb03d45 Part 1: Generalize permission handling of storage-access to all permissions r=whimboo,webdriver-reviewers https://hg.mozilla.org/integration/autoland/rev/c0bc6a564dde Part 2: Adjust notification tests r=asuth,dom-worker-reviewers https://hg.mozilla.org/integration/autoland/rev/5df20aa207c2 Part 3: Remove dom.screenwakelock.testing r=dom-core,edgar
Upstream PR was closed without merging

Builds were broken already before so probably not a problem with this patch?

I think it's more about the android crash, which still is a thing 🥲

Priority: -- → P2
Whiteboard: [webdriver:m10:external]
Depends on: 1878741

Alright, I think I'll skip the notification side here and do it on bug 1878741 instead as things are complex there than I thought.

Attachment #9373503 - Attachment is obsolete: true
Pushed by krosylight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/08902298c277 Part 1: Generalize permission handling of storage-access to all permissions r=whimboo,webdriver-reviewers https://hg.mozilla.org/integration/autoland/rev/b3966d5a4d29 Part 3: Remove dom.screenwakelock.testing r=dom-core,edgar
Attachment #9373503 - Attachment is obsolete: false

https://hg.mozilla.org/mozilla-central/rev/08902298c277
https://hg.mozilla.org/mozilla-central/rev/b3966d5a4d29

Status: ASSIGNED → RESOLVED
Closed: 8 months ago
status-firefox124: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 124 Branch

Comment on attachment 9373503 [details]
Bug 1875257 - Part 2: Adjust notification tests r=asuth,#dom-worker-reviewers

Revision D198957 was moved to bug 1878741. Setting attachment 9373503 [details] to obsolete.

Attachment #9373503 - Attachment is obsolete: true
Whiteboard: [webdriver:m10:external] → [webdriver:m10][webdriver:external]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: