1152784 - web-platform-tests /eventsource/eventsource-constructor-document-domain.htm (somewhat intermittently) asserts in debug builds

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[James Graham [:jgraham]]

There's a log in https://treeherder.mozilla.org/logviewer.html#?job_id=125821&repo=cedar. The top of the stack looks like:

20:03:12     INFO -  0  libxul.so!xpc::AccessCheck::isCrossOriginAccessPermitted(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, unsigned int) [AccessCheck.cpp:b52cd1d48e79 : 119 + 0x22]
20:03:12     INFO -     eip = 0xb28b98f8   esp = 0xbfa6bbd0   ebp = 0xbfa6bce8   ebx = 0xb6d6d60c
20:03:12     INFO -     esi = 0xaccee040   edi = 0xa9845180   eax = 0x00000000   ecx = 0xb758a8ac
20:03:12     INFO -     edx = 0x00000000   efl = 0x00210286
20:03:12     INFO -     Found by: given as instruction pointer in context
20:03:12     INFO -  1  libxul.so!xpc::FilteringWrapper<xpc::CrossOriginXrayWrapper, xpc::CrossOriginAccessiblePropertiesOnly>::enter(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, unsigned int, bool*) const [AccessCheck.h:b52cd1d48e79 : 80 + 0xb]
20:03:12     INFO -     eip = 0xb28b9e37   esp = 0xbfa6bcf0   ebp = 0xbfa6bd18   ebx = 0xb6d6d60c
20:03:12     INFO -     esi = 0xbfa6bdb9   edi = 0xaccee040
20:03:12     INFO -     Found by: call frame info
20:03:12     INFO -  2  libxul.so!js::AutoEnterPolicy::AutoEnterPolicy [Proxy.h:b52cd1d48e79 : 609 + 0x1e]
20:03:12     INFO -     eip = 0xb4cd7c78   esp = 0xbfa6bd20   ebp = 0xbfa6bd58   ebx = 0xb6d6d60c
20:03:12     INFO -     esi = 0xbfa6bdb4   edi = 0xaccee040
20:03:12     INFO -     Found by: call frame info
20:03:12     INFO -  3  libxul.so!js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) [Proxy.cpp:b52cd1d48e79 : 275 + 0x20]
20:03:12     INFO -     eip = 0xb4cd8601   esp = 0xbfa6bd60   ebp = 0xbfa6bdf8   ebx = 0xb6d6d60c
20:03:12     INFO -     esi = 0xb6dbc908   edi = 0xaccee040
20:03:12     INFO -     Found by: call frame info
20:03:12     INFO -  4  libxul.so!js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) [NativeObject.h:b52cd1d48e79 : 1416 + 0x20]
20:03:12     INFO -     eip = 0xb4620337   esp = 0xbfa6be00   ebp = 0xbfa6be38   ebx = 0xb6d6d60c
20:03:12     INFO -     esi = 0xbfa6c0d8   edi = 0xaccee040
20:03:12     INFO -     Found by: call frame info
20:03:12     INFO -  5  libxul.so!Interpret [Interpreter.cpp:b52cd1d48e79 : 268 + 0x12]
20:03:12     INFO -     eip = 0xb47d0495   esp = 0xbfa6be40   ebp = 0xbfa6c268   ebx = 0xb6d6d60c
20:03:12     INFO -     esi = 0xbfa6c0d8   edi = 0xaccee040
20:03:12     INFO -     Found by: call frame info
20:03:12     INFO -  6  libxul.so!js::RunScript(JSContext*, js::RunState&) [Interpreter.cpp:b52cd1d48e79 : 654 + 0x6]
20:03:12     INFO -     eip = 0xb47daf24   esp = 0xbfa6c270   ebp = 0xbfa6c2f8   ebx = 0xb6d6d60c
20:03:12     INFO -     esi = 0xaccee040   edi = 0xbfa6c2b0
20:03:12     INFO -     Found by: call frame info
20:03:12     INFO -  7  libxul.so!js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) [Interpreter.cpp:b52cd1d48e79 : 723 + 0xe]
20:03:12     INFO -     eip = 0xb47dd489   esp = 0xbfa6c300   ebp = 0xbfa6c698   ebx = 0xb6d6d60c
20:03:12     INFO -     esi = 0x00000000   edi = 0xaccee040
20:03:12     INFO -     Found by: call frame info
20:03:12     INFO -  8  libxul.so!js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) [Interpreter.cpp:b52cd1d48e79 : 760 + 0x2e]
20:03:12     INFO -     eip = 0xb47de33e   esp = 0xbfa6c6a0   ebp = 0xbfa6c798   ebx = 0xb6d6d60c
20:03:12     INFO -     esi = 0xbfa6c718   edi = 0xbfa6c6fc
20:03:12     INFO -     Found by: call frame info
20:03:12     INFO -  9  libxul.so!JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) [jsapi.cpp:b52cd1d48e79 : 4375 + 0x26]
20:03:12     INFO -     eip = 0xb4be22f4   esp = 0xbfa6c7a0   ebp = 0xbfa6c7f8   ebx = 0xb6d6d60c
20:03:12     INFO -     esi = 0xaccee040   edi = 0xbfa6c870
20:03:12     INFO -     Found by: call frame info
20:03:12     INFO - 10  libxul.so!mozilla::dom::Function::Call(JSContext*, JS::Handle<JS::Value>, nsTArray<JS::Value> const&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) [FunctionBinding.cpp:b52cd1d48e79 : 37 + 0x7]
20:03:12     INFO -     eip = 0xb329e35e   esp = 0xbfa6c800   ebp = 0xbfa6c908   ebx = 0xb6d6d60c
20:03:12     INFO -     esi = 0xbfa6c858   edi = 0xbfa6c870
20:03:12     INFO -     Found by: call frame info
20:03:12     INFO - 11  libxul.so!nsGlobalWindow::RunTimeoutHandler(nsTimeout*, nsIScriptContext*) [FunctionBinding.h:b52cd1d48e79 : 55 + 0x1c]
20:03:12     INFO -     eip = 0xb2c6255f   esp = 0xbfa6c910   ebp = 0xbfa6cab8   ebx = 0xb6d6d60c
20:03:12     INFO -     esi = 0xbfa6c970   edi = 0x7d2a6500
20:03:12     INFO -     Found by: call frame info

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

This sounds rather bad.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Uh, so... the code we're asserting at should only be reached when doing property access on a cross-origin window.  Why are we doing that at all in this test?

Comment 3

[Bobby Holley (:bholley)]

Attachment: Be more robust about possible intermediate wrappers in IsFrameId. v1

Boris and I debugged this. It looks like we're somehow ending up with an
XrayWaiver on the other end of a CrossOriginXrayWrapper. The specifics of how
this happens are a bit fuzzy to me, but it's presumably happening in all the
brain transplant weirdness we do when recomputing wrappers during document.domain.

Having an XrayWaiver there isn't unsafe - the wrapper computation algorithm
will ignore the waiver if the principals don't allow the caller to waive. But
it does through a wrench in some brittle code that only expects certain kinds
of wrappers. Let's just do what XrayTraits::getTargetObject does. I don't think
this is really unsafe at all, because the only wrapper with a security boundary
is the CCW, and we're already stripping that off unconditionally with
Wrapper::wrappedObject.

Comment 4

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=98f11075a5b1

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8590474 [details] [diff] [review]
Be more robust about possible intermediate wrappers in IsFrameId. v1

r=me but I'd still like to understand what's going on here.  When running not under a debugger, the first call to this code works ok, but the second call has this xray-for-waiver thing going on.  When running under a debugger, it all works all the time.  That _really_ bothers me.

Comment 6

[Bobby Holley (:bholley)]

There was actually another caller of IdentifyCrossOriginObject, so needed to revert that bit:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=34eb5002406a

(In reply to Not doing reviews right now from comment #5)
> r=me but I'd still like to understand what's going on here.  When running
> not under a debugger, the first call to this code works ok, but the second
> call has this xray-for-waiver thing going on.  When running under a
> debugger, it all works all the time.  That _really_ bothers me.

My assumption is that the waiver is coming from the test harness (since it certainly can't come from web content), and my guess is that the way the harness attaches the debugger alters the something such that we don't hit this condition. I'd definitely be interested to know the details, but I speculate that the benefit of knowing would not outweigh the time spent. If you disagree, you're welcome to poke at it.

Comment 7

[Bobby Holley (:bholley)]

This message from the logs is probably relevant:

 0:16.59 PROCESS_OUTPUT: ProcessReader (pid:62428) "JavaScript error: http://web-platform.test:8000/resources/testharness.js, line 75: Error: Permission denied to access property "test_state_callback""

Comment 8

[Boris Zbarsky [:bzbarsky]]

That message from the logs is surely relevant.  In the crashing case it happens once and then we hit the assert.  In the debugger case we keep getting that message until the test times out.

So it looks like our xray-for-waiver is coming from a .win get on ... something.  The something has an xpc::WaiveXrayWrapper proxy wrapped around an XrayWaiver proxy around an OuterWindowProxy.  So we land in WrapperFactory::WaiveXrayAndWrap with two same-origin principals: the one for http://web-platform.test:8000/testharness_runner.html and the one for http://web-platform.test:8000/eventsource/eventsource-constructor-document-domain.htm.  Except the latter has set document.domain, so they're not _actually_ same-origin; they just pass the subsumes() check that means we put in an XrayWaiver there.

Which just pushes the question one back: how do we have an XrayWaiver _there_?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Also, s/through/throw/ in the commit message when talking about wrenches?

Comment 10

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/21f313bee0aa

Comment 11

[Bobby Holley (:bholley)]

(In reply to Not doing reviews right now from comment #8)
> That message from the logs is surely relevant.  In the crashing case it
> happens once and then we hit the assert.  In the debugger case we keep
> getting that message until the test times out.
> 
> So it looks like our xray-for-waiver is coming from a .win get on ...
> something.  The something has an xpc::WaiveXrayWrapper proxy wrapped around
> an XrayWaiver proxy around an OuterWindowProxy.  So we land in
> WrapperFactory::WaiveXrayAndWrap with two same-origin principals: the one
> for http://web-platform.test:8000/testharness_runner.html and the one for
> http://web-platform.test:8000/eventsource/eventsource-constructor-document-
> domain.htm.  Except the latter has set document.domain, so they're not
> _actually_ same-origin; they just pass the subsumes() check that means we
> put in an XrayWaiver there.
> 
> Which just pushes the question one back: how do we have an XrayWaiver
> _there_?

By "_there_" I assume you mean the "something" above, which presumably lives in testharness_runner.html? I'm assuming that it's getting passed somehow from the marionette bits. I would think that we should strip off that waiver, since we must be passing it from System Principal code (which would be able to make the waiver) to non-system principal code (testharness_runner.html). But the situation may be more complex somehow.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> By "_there_" I assume you mean the "something" above

Yes.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/21f313bee0aa