Bugzilla

Assignee

Comment 3

•

3 years ago

This might be some kind of issue with the JS debugger not dealing with remote window proxies. I'm not sure what might have changed recently that would set that off.

Jan de Mooij [:jandem]

Comment 4

•

3 years ago

We call ToWindowProxyIfWindow on a Window object (IsWindow returns true). Then we unwrap that (to handle CCWs) and check for dead wrappers. After that we expect the object to be a WindowProxy but it isn't...

This happens I think because we call unsafeDereference() on a Debugger.Object wrapper for a global/window?

Comment 5

•

3 years ago

After that we expect the object to be a WindowProxy but it isn't...

Hmm. I think at this point the object could be a remote windowproxy instead of a normal WindowProxy there.

I've tried to reproduce this today on the revision in question (930ad6def3c7) and haven't managed to yet.

I'm going to see whether I can get it in Pernosco, perhaps.

Comment 6

•

3 years ago

OK, with some help from Kyle I got this in Pernosco. https://pernos.co/debug/1Stv_5P3ZP-Z8mQz4iYkDg/index.html#f{m[B3Yv,Eg0/_,t[AjQ,CYE_,f{e[B3Yv,EgnW_,s{afxmNrgAA,bASE,oHaExqg,uHY04Kg___ for a debugging session with some notes attached.

And yes, we are trying to unsafeDereference() a Debugger.Object for different-compartment Window, which is not the active window in its browsing context, and the active window is in another process. So when we to ToWindowProxyIfWindow on it we get a RemoteOuterWindowProxy and that fails the IsWindowProxy assert.

I guess we should do the following:

Audit all callsites of ToWindowProxyIfWindow to make sure they're OK with getting a RemoteOuterWindowProxy.
Audit the downstream wrap code in particular to make sure it's ok with getting a RemoteOuterWindowProxy.
Write a testcase. I'm not sure whether there's a non-debugger way to get here; it's not common for Gecko code to be storing a Window to start with, much less to then try to wrap it into a different compartment. Web IDL APIs that are supposed to return a WindowProxy return the browsing context, not an nsGlobalWindowInner, so can't reach this state.

Assignee

Updated

•

3 years ago

Fission Milestone: --- → ?

Comment 7

•

3 years ago

I just checked the uses of ToWindowProxyIfWindow in dom/ and they are fine.

Comment hidden (Intermittent Failures Robot)

Updated

•

3 years ago

Keywords: intermittent-failure

Chris Peterson [:cpeterson]

Updated

•

3 years ago

Summary: Perma GECKO(1115) | Assertion failure: IsWindowProxy(obj), at /builds/worker/workspace/build/src/js/src/vm/Compartment.cpp:247 → Intermittent [tier2] GECKO(1115) | Assertion failure: IsWindowProxy(obj), at /builds/worker/workspace/build/src/js/src/vm/Compartment.cpp:247

Comment hidden (Intermittent Failures Robot)

Comment 10

•

3 years ago

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #6)

Audit all callsites of ToWindowProxyIfWindow to make sure they're OK with getting a RemoteOuterWindowProxy.

Audit the downstream wrap code in particular to make sure it's ok with getting a RemoteOuterWindowProxy.

Andrew, can you please audit the WindowProxy functions bz suggested in comment 6 (or recommend someone else)?

kmag suspects this is debugger bug, possibly related to transplanted windows wrapped in debugger objects.

Nika suggests this bug block Fission Nightly (M6).

Fission Milestone: ? → M6

status-firefox73: --- → disabled

status-firefox74: --- → affected

Flags: needinfo?(continuation)

Priority: P5 → P3

Comment 11

•

3 years ago

kmag suspects this is debugger bug, possibly related to transplanted windows wrapped in debugger objects.

There's not need to "suspect" that. You can just explicitly see it in the Pernosco trace linked in comment 6. Or could when it still existed...

Comment hidden (Intermittent Failures Robot)

Assignee

Comment 13

•

3 years ago

Sure, I can add this onto my list of things to do.

Flags: needinfo?(continuation)

Comment hidden (Intermittent Failures Robot)

Cosmin Sabou [:CosminS]

Comment 17

•

3 years ago

This is almost permafailing on mochitest with fission devtools jobs: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&searchStr=linux%2C18.04%2Cx64%2Cdebug%2Cmochitests%2Cwith%2Cfission%2Cenabled%2Ctest-linux1804-64%2Fdebug-mochitest-devtools-chrome-fis-e10s-2%2Cm-fis%28dt2%29&fromchange=e84a0546e6e2e1f7a7b5912c86e3e3cab311555c&tochange=ca97340e64722330f45043b3fa8a819ad7c59835&selectedJob=288783858
Recent failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=288904361&repo=mozilla-central
Andrew, could you take a look over it? Thank you.

Flags: needinfo?(continuation)

Assignee

Comment 18

•

3 years ago

Sure, I'll look. You could try disabling devtools/client/webconsole/test/browser/browser_webconsole_warning_group_multiples.js under Fission in the meanwhile. The failures look like they are happening with that test.

Flags: needinfo?(continuation)

Assignee

Comment 19

•

3 years ago

Looks like it might actually be happening during devtools/client/webconsole/test/browser/browser_webconsole_block_mixedcontent_securityerrors.js? I'm not sure. I haven't been able to reproduce it locally yet.

Comment 20

•

3 years ago

(In reply to Andrew McCreight [:mccr8] from comment #19)

Looks like it might actually be happening during devtools/client/webconsole/test/browser/browser_webconsole_block_mixedcontent_securityerrors.js? I'm not sure. I haven't been able to reproduce it locally yet.

Looked in ~20 failure logs, might be more, and assertion occurs in browser_webconsole_block_mixedcontent_securityerrors.js

Cristian Brindusan [:cbrindusan]

Comment 21

•

3 years ago

Attached file Bug 1604593 - disable browser_webconsole_block_mixedcontent_securityerrors.js on fission r=mccr8 — Details

Phabricator Automation

Updated

•

3 years ago

Assignee: nobody → apavel

Status: NEW → ASSIGNED

Pulsebot

Comment 22

•

3 years ago

Pushed by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ac6edc02f133
disable browser_webconsole_block_mixedcontent_securityerrors.js on fission r=mccr8

Comment 23

•

3 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/ac6edc02f133

Status: ASSIGNED → RESOLVED

Closed: 3 years ago

status-firefox75: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla75

Cosmin Sabou [:CosminS]

Comment 24

•

3 years ago

browser_webconsole_block_mixedcontent_securityerrors.js was only disabled on fission here so re-opening.

Assignee: apavel → nobody

Status: RESOLVED → REOPENED

status-firefox75: fixed → ---

Keywords: leave-open

Resolution: FIXED → ---

Whiteboard: [stockwell disabled]

Target Milestone: mozilla75 → ---

Comment hidden (Intermittent Failures Robot)

Assignee

Comment 26

•

3 years ago

I'm trying to understand what is going wrong here. Boris's comment 6 about how we should audit call sites for ToWindowProxyIfWindow sounds reasonable to me, but I don't understand how it helps us with this particular crash.

Maybe what is happening here is that we have an outer window O with an inner window I. This means that O is the window proxy of I. Then we navigate O to some other origin that causes it to be moved out of process. This makes us do a brain transplant on O, so that O is now a remote outer window, which means that the window proxy field of I is now a remote window. That seems like it doesn't make much sense. I think that normally this doesn't matter because nobody has a direct reference to an inner window, but apparently these debugger objects do.

Would we have nuked the realm of I on a navigation that didn't make us change processes? That would make js::AllowNewWrapper() return false. It looks like we do call FreeInnerObjects() on the old inner in SetNewDocument(), which would nuke the old inner eventually (if it isn't being bfcached).

Maybe js::AllowNewWrapper() should detect the case where an inner window has a remote window proxy as its "outer". That seems bad.

Comment 27

•

3 years ago

which means that the window proxy field of I is now a remote window.

Yep.

That seems like it doesn't make much sense.

I think our options are to either make sure this does not happen or to make consumers deal with it. The latter is where auditing ToWindowProxyIfWindow callsites comes in.

Would we have nuked the realm of I on a navigation that didn't make us change processes?

Generally no. Also, if I is bfcached, then when we go back we're going to re-brain-transplant and end up with a real WindowProxy there again, right? Modulo whatever the fission/bfcache interaction is.

which would nuke the old inner eventually

Where?

Assignee

Comment 28

•

3 years ago

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #27)

I think our options are to either make sure this does not happen or to make consumers deal with it. The latter is where auditing ToWindowProxyIfWindow callsites comes in.

Ah, I see. It just feels kind of bogus to call a function that purports to return the window proxy for an inner, but in effect returns a window proxy for some other inner. I wouldn't be surprised if there's some weird behavior in the debugger here in the non-Fission case, but maybe it just doesn't cause issues.

Would we have nuked the realm of I on a navigation that didn't make us change processes?

Generally no. Also, if I is bfcached, then when we go back we're going to re-brain-transplant and end up with a real WindowProxy there again, right? Modulo whatever the fission/bfcache interaction is.

It does seem like we would, in the window destroyed event, at least in the non-BFcached case. But that doesn't happen synchronously, in any event, so we can't rely on it.

which would nuke the old inner eventually

Where?

WindowDestroyedEvent::Run().

Comment 29

•

3 years ago

WindowDestroyedEvent::Run().

Ah, yes. That still wouldn't nuke content->content references into the old window, which could still call functions from it, etc, right?

Assignee

Comment 30

•

3 years ago

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #29)

Ah, yes. That still wouldn't nuke content->content references into the old window, which could still call functions from it, etc, right?

For this specific case, I don't believe that matters. Nuking the realm sets the nukedIncomingWrappers flag, which means js::AllowNewWrapper() will fail and getNonWrapperObjectForCurrentCompartment() will return a dead proxy object instead of a window proxy. I guess that doesn't help the other users of ToWindowProxyIfWindow() if they don't call AllowNewWrapper.

Timothy Nikkel (:tnikkel)

Assignee

Comment 31

•

3 years ago

I spent probably too much time trying to get my head around how inner(outer(i)) != i but I guess that's okay enough.

I think the fix for this immediate issue is just to weaken the assert to accept either a window proxy or a remote window proxy.

I've started auditing the other call sites, but I haven't finished yet. For a lot of them, it seems like something weird and wrongish will happen if they get a remote window proxy, but not really any more wrong than if it had happened in the non-Fission case and they just got an outer for some other inner.

Comment 32

•

3 years ago

FWIW, there seems to be another failure mode for this

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=ecff61b46d80759496eca644c6db82d1b5ebf3c3&selectedJob=289746345

hitting this assert

https://searchfox.org/mozilla-central/rev/5a10be606f2d76ef22f1f44565749490de991d35/docshell/base/BrowsingContext.cpp#1274

Assignee

Comment 33

•

3 years ago

That's on file as bug 1588284. I don't know if it is related to this or not, but it is certainly suspicious that it is in the same test.

Assignee

Updated

•

3 years ago

Comment 34

•

3 years ago

I started thinking about how to test this. Bug 1223372 added a chrome test test_windowProxyDeadWrapper.html, but it doesn't seem to actually hit the code that the patch in that bug added any more. The original STR from that bug, running devtools/shared/webconsole/test/chrome/, does seem to hit it at least.

Assignee

Comment 35

•

3 years ago

I figured out a way to trip the assertion, though it is pretty horrible. I modified file_postMessage_parent.html, which is the page loaded in the iframe in the Fission transplant test browser_windowProxy_transplant.js. I used SpecialPowers to listen for "inner-window-nuked" (which should happen after we've navigated the iframe out of process), and then run SpecialPowers.Cu.getGlobalForObject({});. That snippet of code, which is taken from test_windowProxyDeadWrapper.html, grabs the global of an object in the current window, then wraps it, which ends up in the scenario Boris outlined in comment 6 and triggers the assert.

Assignee

Comment 36

•

3 years ago

I audited callers of ToWindowProxyIfWindow(). Boris said the DOM uses are okay.

JavaScriptShared::findObjectById(): If you have a CPOW target object with an Xray waiver, it calls ToWindowProxyIfWindow() on it, then wraps it. I guess that's okay.
ScriptedCallerGlobal: This is a testing function used for JIT tests. It looks like it won't crash if we return a remote window proxy, but the result surely can't qualify as a "scripted caller global". The tests involved don't seem to do any kind of navigation, Fission or otherwise, so maybe that's okay. (The result is probably not really valid in either case.)
ObjectGlobal and FirstGlobalInCompartment: These look like the same basic situation as ScriptedCallerGlobal.
ValueToWindowProxyIfWindow: used to forward to the proxy in Proxy::get and Proxy::set. I'd guess that this will throw an error if the receiver is an inner that has an "outer" that is remote, but that's probably what we want to have happen.
NewGlobal in the shell: This gets passed a freshly created global, so no issues there.
dom_get_global in the shell: Similar issues as ScriptedCallerGlobal above.
Other caller in Compartment::getNonWrapperObjectForCurrentCompartment(): won't immediately fail at least. called by wrap and rewrap.
GlobalObject::maybeResolveGlobalThis(): This will try to call js::DefineDataProperty on the remote window proxy. Which I think/hope should fail and throw some kind of exception? Probably similar behavior to the non-Fission case?
js::SetPrototype(): This is trying to prevent "cyclical prototype chains" by checking if the outer is on the proto chain(?). I'm not sure if it ever can be, but feels reasonable in either case.
js::GetThisValue(): "this" can end up as a remote window proxy, but I guess that's okay.
nsXPCComponents_Utils::GetGlobalForObject: Just passed back to JS, so okay I guess.
Callers of Compartment::getNonWrapperObjectForCurrentCompartment(): wrap, rewrap. rewrap looked okay.
Callers of wrap(): Seemed okay. One of the callers is JS_WrapObject(), which has over 1000 callers, mostly in bindings code. I spot checked a few of them, and they looked okay.

Assignee

Comment 37

•

3 years ago

Attached file Bug 1604593 - Weaken assert in getNonWrapperObjectForCurrentCompartment(). — Details

If we try to wrap an inner whose outer has navigated out of process,
then we'll get a remote window proxy, not a window proxy.

This was happening during the JS devtools test
browser_webconsole_block_mixedcontent_securityerrors.js

Phabricator Automation

Updated

•

3 years ago

Assignee: nobody → continuation

https://hg.mozilla.org/mozilla-central/rev/77174cb8538c

Comment 38

•

3 years ago

I'm not sure if it ever can be [re proto chains]

It can't be in a cyclic one, because window and windowproxy and remotewindowproxy all have immutable prototypes.

As long as the rest of the wrap() algorithm is ok with having a remotewindowproxy here, sounds like things are fine.

Pulsebot

Comment 39

•

3 years ago

Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/77174cb8538c
Weaken assert in getNonWrapperObjectForCurrentCompartment(). r=bzbarsky

Oana Pop-Rus

Comment 40

•

3 years ago

bugherder