Closed Bug 1507479 Opened 6 years ago Closed 6 years ago

Make WorkerPrivate::mIsSecureContext const

Categories

(Core :: DOM: Workers, defect, P3)

Product:
Core
Component:
DOM: Workers
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla65
Tracking Flags:
Tracking Status
firefox65 --- fixed

People

(Reporter: ytausky, Assigned: ytausky)

References

Details

Attachments

(1 file)

Bug 1507479: Make WorkerPrivate::mIsSecureContext const
47 bytes, text/x-phabricator-request
Details | Review
This member is never changed; the only reason it's not const already is because of some non-idiomatic code in WorkerPrivate's constructor.
This member was never changed after it was set, but it couldn't be const because WorkerLoadInfo used a custom StealFrom() function instead of an idiomatic move constructor and move assignment operator. This commit replaces StealFrom with its idiomatic counterparts, adds a function that determines a new worker's secure context status, and uses that function to initialize the now const mIsSecureContext. Making constant members const is part of a greater effort to ensure that data is either modified from a single thread only, or access to it is synchronized properly.
Keywords: checkin-needed
Pushed by apavel@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7efd8b5c5b93 Make WorkerPrivate::mIsSecureContext const r=perry,asuth
Keywords: checkin-needed
Backed out for asserting in caps/nsJSPrincipals.cpp: https://hg.mozilla.org/integration/autoland/rev/3269a8661fdbf3eafb35dcca6f46a43346e2c779 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&group_state=expanded&revision=7efd8b5c5b934ba3897fb33ac19720c07f4d4057 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=212703498&repo=autoland [task 2018-11-19T20:52:42.259Z] 20:52:42 INFO - TEST-START | dom/broadcastchannel/tests/test_broadcastchannel_worker.html [task 2018-11-19T20:52:42.283Z] 20:52:42 INFO - GECKO(1529) | [Child 1628, Main Thread] WARNING: NS_ENSURE_TRUE(!mHasOrHasHadOwnerWindow || mOwnerWindow) failed: file /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/DOMEventTargetHelper.h, line 145 [task 2018-11-19T20:52:42.461Z] 20:52:42 INFO - GECKO(1529) | ++DOMWINDOW == 19 (0x7f870011a800) [pid = 1628] [serial = 31] [outer = 0x7f8701551400] [task 2018-11-19T20:52:42.563Z] 20:52:42 INFO - GECKO(1529) | --DOCSHELL 0x7ff20877a800 == 7 [pid = 1529] [id = {d9dfbc78-a67e-41e1-b702-b0488fb55079}] [task 2018-11-19T20:52:42.563Z] 20:52:42 INFO - GECKO(1529) | --DOCSHELL 0x7ff20b066000 == 6 [pid = 1529] [id = {d6a44ffc-321f-4f67-8df3-c7f2c51c6c9b}] [task 2018-11-19T20:52:42.640Z] 20:52:42 INFO - GECKO(1529) | [Child 1628, Main Thread] WARNING: NS_ENSURE_TRUE(!mHasOrHasHadOwnerWindow || mOwnerWindow) failed: file /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/DOMEventTargetHelper.h, line 145 [task 2018-11-19T20:52:42.641Z] 20:52:42 INFO - GECKO(1529) | [Child 1628, Main Thread] WARNING: NS_ENSURE_TRUE(!mHasOrHasHadOwnerWindow || mOwnerWindow) failed: file /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/DOMEventTargetHelper.h, line 145 [task 2018-11-19T20:52:42.641Z] 20:52:42 INFO - GECKO(1529) | [Child 1628, Main Thread] WARNING: NS_ENSURE_TRUE(!mHasOrHasHadOwnerWindow || mOwnerWindow) failed: file /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/DOMEventTargetHelper.h, line 145 [task 2018-11-19T20:52:42.642Z] 20:52:42 INFO - GECKO(1529) | --DOCSHELL 0x7f8700218000 == 2 [pid = 1628] [id = {c486f268-9349-409a-8105-057c2f73b4cb}] [task 2018-11-19T20:52:42.643Z] 20:52:42 INFO - GECKO(1529) | --DOMWINDOW == 18 (0x7f8700119800) [pid = 1628] [serial = 20] [outer = (nil)] [url = http://mochi.test:8888/tests/dom/broadcastchannel/tests/iframe_broadcastchannel.html] [task 2018-11-19T20:52:42.679Z] 20:52:42 INFO - GECKO(1529) | Assertion failure: NS_IsMainThread(), at /builds/worker/workspace/build/src/caps/nsJSPrincipals.cpp:30 [task 2018-11-19T20:54:07.160Z] 20:54:07 INFO - GECKO(1529) | #01: mozilla::ContentPrincipal::QueryInterface(nsID const&, void**) [caps/ContentPrincipal.cpp:0] [task 2018-11-19T20:54:07.160Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.161Z] 20:54:07 INFO - GECKO(1529) | #02: nsCOMPtr<nsIPrincipal>::Assert_NoQueryNeeded() [xpcom/base/nsCOMPtr.h:499] [task 2018-11-19T20:54:07.161Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.162Z] 20:54:07 INFO - GECKO(1529) | #03: nsCOMPtr<nsIPrincipal>::operator=(nsCOMPtr<nsIPrincipal>&&) [xpcom/base/nsCOMPtr.h:737] [task 2018-11-19T20:54:07.163Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.164Z] 20:54:07 INFO - GECKO(1529) | #04: mozilla::dom::WorkerLoadInfoData::operator=(mozilla::dom::WorkerLoadInfoData&&) [dom/workers/WorkerLoadInfo.h:114] [task 2018-11-19T20:54:07.170Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.171Z] 20:54:07 INFO - GECKO(1529) | #05: mozilla::dom::WorkerPrivate::GetLoadInfo(JSContext*, nsPIDOMWindowInner*, mozilla::dom::WorkerPrivate*, nsTSubstring<char16_t> const&, bool, mozilla::dom::WorkerPrivate::LoadGroupBehavior, mozilla::dom::WorkerType, mozilla::dom::WorkerLoadInfo*) [dom/workers/WorkerLoadInfo.h:0] [task 2018-11-19T20:54:07.171Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.173Z] 20:54:07 INFO - GECKO(1529) | #06: mozilla::dom::WorkerPrivate::Constructor(JSContext*, nsTSubstring<char16_t> const&, bool, mozilla::dom::WorkerType, nsTSubstring<char16_t> const&, nsTSubstring<char> const&, mozilla::dom::WorkerLoadInfo*, mozilla::ErrorResult&) [dom/bindings/ErrorResult.h:361] [task 2018-11-19T20:54:07.174Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.174Z] 20:54:07 INFO - GECKO(1529) | #07: mozilla::dom::Worker::Constructor(mozilla::dom::GlobalObject const&, nsTSubstring<char16_t> const&, mozilla::dom::WorkerOptions const&, mozilla::ErrorResult&) [dom/workers/Worker.cpp:26] [task 2018-11-19T20:54:07.176Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.176Z] 20:54:07 INFO - GECKO(1529) | #08: mozilla::dom::Worker_Binding::_constructor(JSContext*, unsigned int, JS::Value*) [mfbt/AlreadyAddRefed.h:144] [task 2018-11-19T20:54:07.177Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.183Z] 20:54:07 INFO - GECKO(1529) | #09: CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) [js/src/vm/Interpreter.cpp:468] [task 2018-11-19T20:54:07.184Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.185Z] 20:54:07 INFO - GECKO(1529) | #10: CallJSNativeConstructor(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) [js/src/vm/Interpreter.cpp:485] [task 2018-11-19T20:54:07.186Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.187Z] 20:54:07 INFO - GECKO(1529) | #11: Interpret(JSContext*, js::RunState&) [js/src/vm/Interpreter.cpp:3453] [task 2018-11-19T20:54:07.188Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.189Z] 20:54:07 INFO - GECKO(1529) | #12: js::RunScript(JSContext*, js::RunState&) [js/src/vm/Interpreter.cpp:447] [task 2018-11-19T20:54:07.190Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.192Z] 20:54:07 INFO - GECKO(1529) | #13: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) [js/src/vm/Interpreter.cpp:587] [task 2018-11-19T20:54:07.193Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.194Z] 20:54:07 INFO - GECKO(1529) | #14: <name omitted> [js/src/vm/Interpreter.cpp:633] [task 2018-11-19T20:54:07.195Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.196Z] 20:54:07 INFO - GECKO(1529) | #15: JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) [js/src/jsapi.cpp:2994] [task 2018-11-19T20:54:07.197Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.198Z] 20:54:07 INFO - GECKO(1529) | #16: mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) [s3:gecko-generated-sources:344e7bbc1d2eb535f4d85b157b525fa18eb1e4688bb70bf6e3c51b02fe597784d3fe3e6a2b078d2e5b5c857d2435e216ad8a578cae88f140c6e1c2a969833ac5/dom/bindings/EventHandlerBinding.cpp::265] [task 2018-11-19T20:54:07.199Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.200Z] 20:54:07 INFO - GECKO(1529) | #17: void mozilla::dom::EventHandlerNonNull::Call<nsISupports*>(nsISupports* const&, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*) [s3:gecko-generated-sources:3760fb8938acb537e28af191191b3b28eae59c21a2b0dcc49f0990b2613d2b29a3f705864d9a9ad5a160e5faa013e2d0b447215b3fc0b25795d7ebbd0dc4ce29/dist/include/mozilla/dom/EventHandlerBinding.h::363] [task 2018-11-19T20:54:07.201Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.203Z] 20:54:07 INFO - GECKO(1529) | #18: mozilla::JSEventHandler::HandleEvent(mozilla::dom::Event*) [dom/events/JSEventHandler.cpp:214] [task 2018-11-19T20:54:07.204Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.205Z] 20:54:07 INFO - GECKO(1529) | #19: mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) [dom/events/EventListenerManager.cpp:1115] [task 2018-11-19T20:54:07.206Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.207Z] 20:54:07 INFO - GECKO(1529) | #20: mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) [dom/events/EventListenerManager.cpp:1317] [task 2018-11-19T20:54:07.208Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.209Z] 20:54:07 INFO - GECKO(1529) | #21: mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) [dom/events/EventDispatcher.cpp:430] [task 2018-11-19T20:54:07.210Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.211Z] 20:54:07 INFO - GECKO(1529) | #22: mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) [dom/events/EventDispatcher.cpp:644] [task 2018-11-19T20:54:07.212Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.213Z] 20:54:07 INFO - GECKO(1529) | #23: mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) [dom/events/EventDispatcher.cpp:1164] [task 2018-11-19T20:54:07.214Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.215Z] 20:54:07 INFO - GECKO(1529) | #24: mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsPresContext*, nsEventStatus*) [dom/events/EventDispatcher.cpp:1252] [task 2018-11-19T20:54:07.217Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.218Z] 20:54:07 INFO - GECKO(1529) | #25: mozilla::DOMEventTargetHelper::DispatchEvent(mozilla::dom::Event&, mozilla::dom::CallerType, mozilla::ErrorResult&) [dom/events/DOMEventTargetHelper.cpp:185] [task 2018-11-19T20:54:07.219Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.220Z] 20:54:07 INFO - GECKO(1529) | #26: mozilla::dom::EventTarget::DispatchEvent(mozilla::dom::Event&) [dom/bindings/ErrorResult.h:809] [task 2018-11-19T20:54:07.221Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.222Z] 20:54:07 INFO - GECKO(1529) | #27: mozilla::dom::MessageEventRunnable::DispatchDOMEvent(JSContext*, mozilla::dom::WorkerPrivate*, mozilla::DOMEventTargetHelper*, bool) [mfbt/RefPtr.h:84] [task 2018-11-19T20:54:07.223Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.224Z] 20:54:07 INFO - GECKO(1529) | #28: mozilla::dom::WorkerRunnable::Run() [dom/workers/WorkerRunnable.cpp:387] [task 2018-11-19T20:54:07.225Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.226Z] 20:54:07 INFO - GECKO(1529) | #29: nsThread::ProcessNextEvent(bool, bool*) [xpcom/threads/nsThread.cpp:1231] [task 2018-11-19T20:54:07.227Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.228Z] 20:54:07 INFO - GECKO(1529) | #30: NS_ProcessNextEvent(nsIThread*, bool) [xpcom/threads/nsThreadUtils.cpp:530] [task 2018-11-19T20:54:07.230Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.231Z] 20:54:07 INFO - GECKO(1529) | #31: mozilla::dom::WorkerPrivate::DoRunLoop(JSContext*) [mfbt/RefPtr.h:307] [task 2018-11-19T20:54:07.232Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.233Z] 20:54:07 INFO - GECKO(1529) | #32: mozilla::dom::workerinternals::(anonymous namespace)::WorkerThreadPrimaryRunnable::Run() [dom/workers/RuntimeService.cpp:2772] [task 2018-11-19T20:54:07.234Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.235Z] 20:54:07 INFO - GECKO(1529) | #33: nsThread::ProcessNextEvent(bool, bool*) [xpcom/threads/nsThread.cpp:1231] [task 2018-11-19T20:54:07.236Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.237Z] 20:54:07 INFO - GECKO(1529) | #34: NS_ProcessNextEvent(nsIThread*, bool) [xpcom/threads/nsThreadUtils.cpp:530] [task 2018-11-19T20:54:07.238Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.239Z] 20:54:07 INFO - GECKO(1529) | #35: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) [ipc/glue/MessagePump.cpp:335] [task 2018-11-19T20:54:07.240Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.242Z] 20:54:07 INFO - GECKO(1529) | #36: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:598] [task 2018-11-19T20:54:07.243Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.244Z] 20:54:07 INFO - GECKO(1529) | #37: nsThread::ThreadFunc(void*) [xpcom/threads/nsThread.cpp:505] [task 2018-11-19T20:54:07.246Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.495Z] 20:54:07 INFO - GECKO(1529) | #38: _pt_root [nsprpub/pr/src/pthreads/ptthread.c:204] [task 2018-11-19T20:54:07.495Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.495Z] 20:54:07 INFO - GECKO(1529) | #39: libpthread.so.0 + 0x76ba [task 2018-11-19T20:54:07.495Z] 20:54:07 INFO - [task 2018-11-19T20:54:07.495Z] 20:54:07 INFO - GECKO(1529) | #40: libc.so.6 + 0x10741d [task 2018-11-19T20:54:07.495Z] 20:54:07 INFO -
Flags: needinfo?(ytausky)
It looks like a certain function [1] that's only called via the move assignment operator in debug mode [2] was calling AddRef() on an object whose refcount can only be accessed from the main thread. The reason it didn't fail before was that the existing code was using swap(), which doesn't call the offending function [3]. Calling this asserting in a move constructor/assignment operator seems like an error to me. I opened bug 1508627 to get that changed. [1] https://searchfox.org/mozilla-central/source/xpcom/base/nsCOMPtr.h#492 [2] https://searchfox.org/mozilla-central/source/xpcom/base/nsCOMPtr.h#736 [3] https://searchfox.org/mozilla-central/source/xpcom/base/nsCOMPtr.h#874
Flags: needinfo?(ytausky)
Depends on: 1508627
Bug 1508627 is fixed and I rebased the patch, let's try again.
Keywords: checkin-needed
Pushed by dluca@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/81a99e4b008c Make WorkerPrivate::mIsSecureContext const r=perry,asuth
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/81a99e4b008c
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Assignee: nobody → ytausky
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: