1040333 - Worker global is set up before a principal is assigned to it

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Reuben Morais [:reuben]]

We only call WorkerPrivate::SetPrincipal after the global has been setup, so for prefable interfaces that depend on principal checks, like something with a [AvailableIn] annotation, the check always fails.

Comment 1

[Reuben Morais [:reuben]]

Stack to IsInPrivilegedApp (and WorkerPrivate::CreateGlobalScope):

    (lldb) bt
    * thread #65: tid = 0x2e8396, 0x000000010b788f1d XUL`mozilla::dom::IsInPrivilegedApp(aCx=0x0000000139df4820, aObj=0x0000000144003060) + 29 at BindingUtils.cpp:2251, name = 'DOM Worker', stop reason = breakpoint 2.1
      * frame #0: 0x000000010b788f1d XUL`mozilla::dom::IsInPrivilegedApp(aCx=0x0000000139df4820, aObj=0x0000000144003060) + 29 at BindingUtils.cpp:2251
        frame #1: 0x000000010b79438d XUL`mozilla::dom::Prefable<JSPropertySpec const>::isEnabled(this=0x00000001117ab848, cx=0x0000000139df4820, obj=0x0000000144004220) const + 333 at DOMJSClass.h:68
        frame #2: 0x000000010b793b46 XUL`bool mozilla::dom::DefinePrefable<JSPropertySpec const>(cx=0x0000000139df4820, obj=Handle<JSObject *> at 0x000000013f9ffc00, props=0x00000001117ab848) + 262 at BindingUtils.cpp:316
        frame #3: 0x000000010b781255 XUL`mozilla::dom::DefineProperties(cx=0x0000000139df4820, obj=Handle<JSObject *> at 0x000000013f9ffc70, properties=0x0000000111515850, chromeOnlyProperties=0x0000000000000000) + 149 at BindingUtils.cpp:621
        frame #4: 0x000000010b781d48 XUL`mozilla::dom::CreateInterfacePrototypeObject(cx=0x0000000139df4820, global=Handle<JSObject *> at 0x000000013f9ffd20, parentProto=Handle<JSObject *> at 0x000000013f9ffd18, protoClass=0x0000000111515540, properties=0x0000000111515850, chromeOnlyProperties=0x0000000000000000) + 216 at BindingUtils.cpp:602
        frame #5: 0x000000010b781996 XUL`mozilla::dom::CreateInterfaceObjects(cx=0x0000000139df4820, global=Handle<JSObject *> at 0x000000013f9ffed0, protoProto=Handle<JSObject *> at 0x000000013f9ffec8, protoClass=0x0000000111515540, protoCache=0x00000001266d66a0, constructorProto=Handle<JSObject *> at 0x000000013f9ffec0, constructorClass=0x00000001115156c8, constructor=0x0000000000000000, ctorNargs=0, namedConstructors=0x0000000000000000, constructorCache=0x00000001266d6710, properties=0x0000000111515850, chromeOnlyProperties=0x0000000000000000, name=0x000000010fdff089, defineOnGlobal=true) + 1526 at BindingUtils.cpp:691
        frame #6: 0x000000010b6ffa6c XUL`mozilla::dom::XMLHttpRequestBinding_workers::CreateInterfaceObjects(aCx=0x0000000139df4820, aGlobal=Handle<JSObject *> at 0x000000013f9ffff0, aProtoAndIfaceCache=0x000000012fbe3ca0, aDefineOnGlobal=true) + 476 at XMLHttpRequestBinding.cpp:3507
        frame #7: 0x000000010b6ffb24 XUL`mozilla::dom::XMLHttpRequestBinding_workers::GetConstructorObject(aCx=0x0000000139df4820, aGlobal=Handle<JSObject *> at 0x000000013fa00050, aDefineOnGlobal=true) + 164 at XMLHttpRequestBinding.cpp:3558
        frame #8: 0x000000010c0351d6 XUL`mozilla::dom::workers::WorkerPrivate::RegisterBindings(this=0x000000010935a800, aCx=0x0000000139df4820, aGlobal=Handle<JSObject *> at 0x000000013fa00220) + 1062 at RegisterBindings.cpp:72
        frame #9: 0x000000010c073faa XUL`mozilla::dom::workers::WorkerPrivate::CreateGlobalScope(this=0x000000010935a800, aCx=0x0000000139df4820) + 682 at WorkerPrivate.cpp:5849
        frame #10: 0x000000010c07e37f XUL`(anonymous namespace)::CompileScriptRunnable::WorkerRun(this=0x000000013dee03a0, aCx=0x0000000139df4820, aWorkerPrivate=0x000000010935a800) + 47 at WorkerPrivate.cpp:886
        frame #11: 0x000000010c092d13 XUL`mozilla::dom::workers::WorkerRunnable::Run(this=0x000000013dee03a0) + 1459 at WorkerRunnable.cpp:312
        frame #12: 0x0000000109ac8aa2 XUL`nsThread::ProcessNextEvent(this=0x0000000139bd8100, aMayWait=false, aResult=0x000000013fa0070e) + 1570 at nsThread.cpp:766
        frame #13: 0x00000001099aefd7 XUL`NS_ProcessNextEvent(aThread=0x0000000139bd8100, aMayWait=false) + 151 at nsThreadUtils.cpp:256
        frame #14: 0x000000010c06a928 XUL`mozilla::dom::workers::WorkerPrivate::DoRunLoop(this=0x000000010935a800, aCx=0x0000000139df4820) + 1656 at WorkerPrivate.cpp:4069
        frame #15: 0x000000010c0400ec XUL`(anonymous namespace)::WorkerThreadPrimaryRunnable::Run(this=0x000000013dee0370) + 716 at RuntimeService.cpp:2725
        frame #16: 0x0000000109ac8aa2 XUL`nsThread::ProcessNextEvent(this=0x0000000139bd8100, aMayWait=false, aResult=0x000000013fa00c4e) + 1570 at nsThread.cpp:766
        frame #17: 0x00000001099aefd7 XUL`NS_ProcessNextEvent(aThread=0x0000000139bd8100, aMayWait=false) + 151 at nsThreadUtils.cpp:256
        frame #18: 0x000000010a107713 XUL`mozilla::ipc::MessagePumpForNonMainThreads::Run(this=0x0000000142f85e80, aDelegate=0x000000012e2b76a0) + 675 at MessagePump.cpp:326
        frame #19: 0x000000010a088776 XUL`MessageLoop::RunInternal(this=0x000000012e2b76a0) + 118 at message_loop.cc:229
        frame #20: 0x000000010a088685 XUL`MessageLoop::RunHandler(this=0x000000012e2b76a0) + 21 at message_loop.cc:222
        frame #21: 0x000000010a08862d XUL`MessageLoop::Run(this=0x000000012e2b76a0) + 45 at message_loop.cc:196
        frame #22: 0x0000000109ac7126 XUL`nsThread::ThreadFunc(aArg=0x0000000139bd8100) + 358 at nsThread.cpp:346
        frame #23: 0x000000010963eccf libnss3.dylib`_pt_root(arg=0x0000000141d3c1c0) + 463 at ptthread.c:212
        frame #24: 0x000000010565d899 libsystem_pthread.dylib`_pthread_body + 138
        frame #25: 0x000000010565d72a libsystem_pthread.dylib`_pthread_start + 137 

And then the stack to WorkerPrivate::SetPrincipal:

(lldb) bt
* thread #1: tid = 0x2e81b4, 0x000000010c08815d XUL`mozilla::dom::workers::WorkerPrivateParent<mozilla::dom::workers::WorkerPrivate>::SetPrincipal(this=0x000000010935a800, aPrincipal=0x0000000132d304c0) + 29 at WorkerPrivate.cpp:3419, queue = 'com.apple.main-thread', stop reason = breakpoint 3.1
  * frame #0: 0x000000010c08815d XUL`mozilla::dom::workers::WorkerPrivateParent<mozilla::dom::workers::WorkerPrivate>::SetPrincipal(this=0x000000010935a800, aPrincipal=0x0000000132d304c0) + 29 at WorkerPrivate.cpp:3419
    frame #1: 0x000000010c04d61a XUL`(anonymous namespace)::ScriptLoaderRunnable::OnStreamCompleteInternal(this=0x0000000139a88e70, aLoader=0x0000000139750bc0, aContext=0x0000000129cc7680, aStatus=NS_OK, aStringLen=1791, aString=0x0000000127138000, aLoadInfo=0x0000000139750b88)::ScriptLoadInfo&) + 3674 at ScriptLoader.cpp:550
    frame #2: 0x000000010c04c656 XUL`(anonymous namespace)::ScriptLoaderRunnable::OnStreamComplete(this=0x0000000139a88e70, aLoader=0x0000000139750bc0, aContext=0x0000000129cc7680, aStatus=NS_OK, aStringLen=1791, aString=0x0000000127138000) + 358 at ScriptLoader.cpp:264
    frame #3: 0x000000010c04c7ad XUL`non-virtual thunk to (anonymous namespace)::ScriptLoaderRunnable::OnStreamComplete(this=0x0000000139a88e80, aLoader=0x0000000139750bc0, aContext=0x0000000129cc7680, aStatus=NS_OK, aStringLen=1791, aString=0x0000000127138000) + 77 at ScriptLoader.cpp:271
    frame #4: 0x0000000109c2924c XUL`nsStreamLoader::OnStopRequest(this=0x0000000139750bc0, request=0x000000010935a058, ctxt=0x0000000129cc7680, aStatus=NS_OK) + 220 at nsStreamLoader.cpp:100
    frame #5: 0x0000000109c2798b XUL`nsStreamListenerTee::OnStopRequest(this=0x00000001302f7c40, request=0x000000010935a058, context=0x0000000129cc7680, status=NS_OK) + 539 at nsStreamListenerTee.cpp:53
    frame #6: 0x0000000109e50a86 XUL`mozilla::net::nsHttpChannel::OnStopRequest(this=0x000000010935a000, request=0x0000000139b23b30, ctxt=0x0000000000000000, status=NS_OK) + 3958 at nsHttpChannel.cpp:5176
    frame #7: 0x0000000109e511bd XUL`non-virtual thunk to mozilla::net::nsHttpChannel::OnStopRequest(this=0x000000010935a3d0, request=0x0000000139b23b30, ctxt=0x0000000000000000, status=NS_OK) + 61 at nsHttpChannel.cpp:5195
    frame #8: 0x0000000109be8f71 XUL`nsInputStreamPump::OnStateStop(this=0x0000000139b23b30) + 1089 at nsInputStreamPump.cpp:711
    frame #9: 0x0000000109be7fbd XUL`nsInputStreamPump::OnInputStreamReady(this=0x0000000139b23b30, stream=0x000000013a09a6b8) + 445 at nsInputStreamPump.cpp:440
    frame #10: 0x0000000109be909f XUL`non-virtual thunk to nsInputStreamPump::OnInputStreamReady(this=0x0000000139b23b38, stream=0x000000013a09a6b8) + 47 at nsInputStreamPump.cpp:492
    frame #11: 0x0000000109a9dba0 XUL`nsInputStreamReadyEvent::Run(this=0x0000000141d17cc0) + 144 at nsStreamUtils.cpp:88
    frame #12: 0x0000000109ac8aa2 XUL`nsThread::ProcessNextEvent(this=0x00000001093219c0, aMayWait=false, aResult=0x00007fff5fbfcbb3) + 1570 at nsThread.cpp:766
    frame #13: 0x00000001099aee3a XUL`NS_ProcessPendingEvents(aThread=0x00000001093219c0, aTimeout=20) + 154 at nsThreadUtils.cpp:198
    frame #14: 0x000000010b989de9 XUL`nsBaseAppShell::NativeEventCallback(this=0x0000000123646520) + 201 at nsBaseAppShell.cpp:98
    frame #15: 0x000000010b913ecd XUL`nsAppShell::ProcessGeckoEvents(aInfo=0x0000000123646520) + 445 at nsAppShell.mm:374
    frame #16: 0x000000010038a5b1 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
    frame #17: 0x000000010037bc62 CoreFoundation`__CFRunLoopDoSources0 + 242
    frame #18: 0x000000010037b3ef CoreFoundation`__CFRunLoopRun + 831
    frame #19: 0x000000010037ae75 CoreFoundation`CFRunLoopRunSpecific + 309
    frame #20: 0x00000001026f3a0d HIToolbox`RunCurrentEventLoopInMode + 226
    frame #21: 0x00000001026f37b7 HIToolbox`ReceiveNextEventCommon + 479
    frame #22: 0x00000001026f35bc HIToolbox`_BlockUntilNextEventMatchingListInModeWithFilter + 65
    frame #23: 0x000000010068a24e AppKit`_DPSNextEvent + 1434
    frame #24: 0x000000010068989b AppKit`-[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 122
    frame #25: 0x000000010b912a47 XUL`-[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:](self=0x0000000123580350, _cmd=0x00000001010bd5c3, mask=18446744073709551615, expiration=0x422d63c37f00000d, mode=0x000000010051ed00, flag='\x01') + 119 at nsAppShell.mm:129
    frame #26: 0x000000010067d99c AppKit`-[NSApplication run] + 553
    frame #27: 0x000000010b914857 XUL`nsAppShell::Run(this=0x0000000123646520) + 167 at nsAppShell.mm:647
    frame #28: 0x000000010d77835c XUL`nsAppStartup::Run(this=0x0000000123536290) + 156 at nsAppStartup.cpp:278
    frame #29: 0x000000010d63254d XUL`XREMain::XRE_mainRun(this=0x00007fff5fbfecb0) + 6205 at nsAppRunner.cpp:4013
    frame #30: 0x000000010d632dc3 XUL`XREMain::XRE_main(this=0x00007fff5fbfecb0, argc=5, argv=0x00007fff5fbff5a0, aAppData=0x00007fff5fbfef30) + 739 at nsAppRunner.cpp:4084
    frame #31: 0x000000010d633272 XUL`XRE_main(argc=5, argv=0x00007fff5fbff5a0, aAppData=0x00007fff5fbfef30, aFlags=0) + 98 at nsAppRunner.cpp:4298
    frame #32: 0x0000000100001eff firefox`do_main(argc=5, argv=0x00007fff5fbff5a0, xreDirectory=0x000000010930e100) + 1839 at nsBrowserApp.cpp:282
    frame #33: 0x0000000100001343 firefox`main(argc=5, argv=0x00007fff5fbff5a0) + 323 at nsBrowserApp.cpp:643

Comment 2

[Reuben Morais [:reuben]]

I'm testing this by adding [AvailableIn="PrivilegedApps"] to the XMLHttpRequest.mozSystem attribute and then running dom/workers/test/test_xhr_parameters.html.

Comment 3

[Reuben Morais [:reuben]]

Attachment: Set principal flags in WorkerPrivateParent ctor

Comment 4

[Reuben Morais [:reuben]]

Attachment: Set principal flags in GetLoadInfo

The other one get the mainthreadedness wrong.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8459076 [details] [diff] [review]
Set principal flags in GetLoadInfo

Review of attachment 8459076 [details] [diff] [review]:
-----------------------------------------------------------------

So why do we need WorkerPrivateParent<Derived>::SetPrincipal at all now?

Comment 6

[Reuben Morais [:reuben]]

There are checks in ScriptLoaderRunnable::OnStreamCompleteInternal to make sure the final principal is same-origin compatible with the load principal (nsIPrincipal.checkMayLoad), and then the worker's principal is updated. It's not useful for the bindings stuff, but dom/workers/ has a few too many calls to GetPrincipal for me to know if that update is useful for something else. I'll remove it and do a Try run to see if something goes red.

Comment 7

[Reuben Morais [:reuben]]

Removing that call breaks all sorts of things: https://tbpl.mozilla.org/?tree=Try&rev=ce1cc9bfdf2c

Comment 8

[Reuben Morais [:reuben]]

See comment 7.

Comment 9

[Reuben Morais [:reuben]]

Thanks!

https://hg.mozilla.org/integration/b2g-inbound/rev/3af5c96cb7b0

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3af5c96cb7b0