Closed Bug 1221852 Opened 4 years ago Closed 4 years ago
Crash [@ mozilla::dom::Shared
Worker Binding::get _port ] after closing a Shared Worker
Debug build: Assertion failure: value, at ../../dist/include/mozilla/dom/BindingUtils.h:966 Nightly: bp-2b9136ce-8bd1-4599-805e-4b07b2151105
Assignee: nobody → amarchesini
Comment on attachment 8683597 [details] [diff] [review] foo.patch serviceworker.port.foo = serviceworker; shouldn't leak with this change, but please test.
Attachment #8683597 - Flags: review?(khuey) → review+
[Security approval request comment] How easily could an exploit be constructed based on the patch? It's relatively easy to reproduce but I don't think it can exploited. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Yes: we return a nullptr in a getter method where we were supposed to return a MessagePort. Which older supported branches are affected by this flaw? all. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Easy to be backported. How likely is this patch to cause regressions; how much testing does it need? I cannot imagine any regression with this patch.
Andrea: If we were returning a null and crashing sometimes, with this patch we'll be handing a pointer to a closed MessagePort back to whoever was crashing. Is that safe?
(In reply to Daniel Veditz [:dveditz] from comment #5) > Andrea: If we were returning a null and crashing sometimes, with this patch > we'll be handing a pointer to a closed MessagePort back to whoever was > crashing. Is that safe? It is. You already can do: var a = new MessageChannel(); a.port1.close(); Or, a port is in a 'closed' state when you transfer it to a different 'context' via postMessage.
Comment on attachment 8683698 [details] [diff] [review] foo.patch This doesn't need sec-approval since it isn't sec-high or sec-critical. Just check it in.
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/62586d115939 for frequent test_sharedWorker_ports.html timeouts: Android: https://treeherder.mozilla.org/logviewer.html#?job_id=17097545&repo=mozilla-inbound B2G emulator: https://treeherder.mozilla.org/logviewer.html#?job_id=17102401&repo=mozilla-inbound Mulet: https://treeherder.mozilla.org/logviewer.html#?job_id=17100021&repo=mozilla-inbound
Hmm, don't we want to fix this in some simple way so that this could land on branches and Bug 1224825 would then land on trunk?
I prefer to land both patch (bug 1224825) in other branches as well. Otherwise we can have intermittent failures.
Had to back this out together with bug 1224825 for M(3) failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/4922c78419fa http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-win32/1447588173/mozilla-inbound_win7-ix_test-mochitest-3-bm126-tests1-windows-build74.txt.gz 05:02:40 WARNING - TEST-UNEXPECTED-FAIL | dom/security/test/csp/test_upgrade_insecure_reporting.html | application terminated with exit code 1 05:02:40 INFO - runtests.py | Application ran for: 0:01:26.973000 05:02:40 INFO - zombiecheck | Reading PID log: c:\users\cltbld\appdata\local\temp\tmpczcznlpidlog 05:02:40 INFO - mozcrash Downloading symbols from: https://queue.taskcluster.net/v1/task/9IEpt2YhSKiHy2Y4angz2w/artifacts/public/build/firefox-45.0a1.en-US.win32.crashreporter-symbols.zip 05:02:51 INFO - mozcrash Saved minidump as C:\slave\test\build\blobber_upload_dir\4a5d6160-41b7-4840-9ee8-2492525fefb6.dmp 05:02:51 INFO - mozcrash Saved app info as C:\slave\test\build\blobber_upload_dir\4a5d6160-41b7-4840-9ee8-2492525fefb6.extra 05:02:51 WARNING - PROCESS-CRASH | dom/security/test/csp/test_upgrade_insecure_reporting.html | application crashed [@ mozilla::`anonymous namespace'::RunWatchdog(void *)]
And out again in https://hg.mozilla.org/integration/mozilla-inbound/rev/a63e2f7359f3, RunWatchdog crashes in desktop mochitest-3 running dom/security/test/csp/test_upgrade_insecure_reporting.html, and if my bet is right also Android mochitest hangs.
Approval Request Comment [Feature/regressing bug #]: SharedWorker [User impact if declined]: a crash [Describe test coverage new/current, TreeHerder]: treeherder [Risks and why]: none [String/UUID change made/needed]: none For beta we don't want to land the test because it introduces a intermittent failure already fixed by bug 1224825. But we don't want to land the patches for bug 1224825 in beta.
I'll wait a day or two, to see if this crash signature is gone in Nightly, before taking it in Aurora.
I don't see the crash signature at all in crash-stats so I'm not sure waiting is going to help us determine anything. But we could verify from the test case. I'll go do that now.
Can the test be added as a crashtest? Perhaps with the timeout replaced with something less timery?
Looks good now on a current build from m-c!
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #21) > I don't see the crash signature at all in crash-stats so I'm not sure > waiting is going to help us determine anything. But we could verify from the > test case. I'll go do that now. Liz, if you look at the first signature and look for data from 28 days, you will see one instance from 11-05 45.0a2 build.
Comment on attachment 8688699 [details] [diff] [review] bar.patch Fix looks simple and safe enough (been on Nightly for 2 days) to uplift to Aurora44.
Attachment #8688699 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Andrea, if you want to leave the tests out do you need to make a new patch for uplift to beta?
Comment on attachment 8689775 [details] [diff] [review] patch without tests [Triage Comment] Crash fix. Patch rebased for beta. OK to uplift.
Attachment #8689775 - Flags: approval-mozilla-beta+
Comment on attachment 8688699 [details] [diff] [review] bar.patch Making the old beta patch obsolete for clarity.
You need to log in before you can comment on or make changes to this bug.