Open Bug 1608087 Opened 6 years ago Updated 3 years ago

Assertion failure: cache->GetWrapperMaybeDead() == obj || (js::RuntimeIsBeingDestroyed() && !cache->GetWrapperMaybeDead())

Categories

(Core :: DOM: Bindings (WebIDL), defect)

Product:
Core
Component:
DOM: Bindings (WebIDL)
Type:
defect

Tracking

()

People

(Reporter: bc, Unassigned)

References

(
URL
)

Details

(Keywords: assertion)

  1. http://.google.hu/ or http://aops..uscg.mil/ using a squid proxy.

Reproduces on Linux and Windows Nightly debug builds. This is pretty old though. I haven't filed before since it appears to require the proxy.

Attempting this without a proxy results in a network error "We can’t connect to the server at ..."

  1. Assertion failure: cache->GetWrapperMaybeDead() == obj || (js::RuntimeIsBeingDestroyed() && !cache->GetWrapperMaybeDead())

Like bug 1566684, I'll start this in GC land.

s-s due to GC.

Assertion failure: cache->GetWrapperMaybeDead() == obj || (js::RuntimeIsBeingDestroyed() && !cache->GetWrapperMaybeDead()), at /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BindingUtils.h:1380
#01: JSObject::finalize(JSFreeOp*) [js/public/Class.h:0]
#02: unsigned long js::gc::Arena::finalize<JSObject>(JSFreeOp*, js::gc::AllocKind, unsigned long) [js/src/gc/GC.cpp:0]
#03: bool FinalizeTypedArenas<JSObject>(JSFreeOp*, js::gc::Arena**, js::gc::SortedArenaList&, js::gc::AllocKind, js::SliceBudget&) [js/src/gc/GC.cpp:535]
#04: js::gc::ArenaLists::foregroundFinalize(JSFreeOp*, js::gc::AllocKind, js::SliceBudget&, js::gc::SortedArenaList&) [js/src/gc/GC.cpp:0]
#05: js::gc::GCRuntime::finalizeAllocKind(JSFreeOp*, js::SliceBudget&) [js/src/gc/GC.cpp:5664]
#06: sweepaction::SweepActionForEach<ContainerIter<mozilla::EnumSet<js::gc::AllocKind, unsigned long> >, mozilla::EnumSet<js::gc::AllocKind, unsigned long> >::run(js::gc::SweepAction::Args&) [js/src/gc/GC.cpp:5890]
#07: sweepaction::SweepActionSequence::run(js::gc::SweepAction::Args&) [js/src/gc/GC.cpp:0]
#08: sweepaction::SweepActionForEach<js::gc::SweepGroupZonesIter, JSRuntime*>::run(js::gc::SweepAction::Args&) [js/src/gc/GC.cpp:0]
#09: sweepaction::SweepActionSequence::run(js::gc::SweepAction::Args&) [js/src/gc/GC.cpp:0]
#10: sweepaction::SweepActionForEach<js::gc::SweepGroupsIter, JSRuntime*>::run(js::gc::SweepAction::Args&) [js/src/gc/GC.cpp:5890]
#11: js::gc::GCRuntime::performSweepActions(js::SliceBudget&) [js/src/gc/GC.cpp:6023]
#12: js::gc::GCRuntime::incrementalSlice(js::SliceBudget&, mozilla::Maybe<JSGCInvocationKind> const&, JS::GCReason, js::gc::AutoGCSession&) [js/src/gc/GC.cpp:6545]
#13: js::gc::GCRuntime::gcCycle(bool, js::SliceBudget, mozilla::Maybe<JSGCInvocationKind> const&, JS::GCReason) [js/src/gc/GC.cpp:6961]
#14: js::gc::GCRuntime::collect(bool, js::SliceBudget, mozilla::Maybe<JSGCInvocationKind> const&, JS::GCReason) [js/src/gc/GC.cpp:7141]
#15: js::gc::GCRuntime::gcSlice(JS::GCReason, long) [js/src/gc/GC.cpp:7238]
#16: nsJSContext::GarbageCollectNow(JS::GCReason, nsJSContext::IsIncremental, nsJSContext::IsShrinking, long) [dom/base/nsJSEnvironment.cpp:1168]
#17: InterSliceGCRunnerFired(mozilla::TimeStamp, void*) [dom/base/nsJSEnvironment.cpp:1744]
#18: mozilla::IdleTaskRunner::Run() [xpcom/threads/IdleTaskRunner.cpp:63]
#19: nsThread::ProcessNextEvent(bool, bool*) [xpcom/threads/nsThread.cpp:1246]
#20: NS_ProcessNextEvent(nsIThread*, bool) [xpcom/threads/nsThreadUtils.cpp:486]
#21: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) [ipc/glue/MessagePump.cpp:87]
#22: MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:315]
#23: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:291]
#24: nsBaseAppShell::Run() [widget/nsBaseAppShell.cpp:139]
#25: XRE_RunAppShell() [toolkit/xre/nsEmbedFunctions.cpp:946]
#26: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) [ipc/glue/MessagePump.cpp:237]
#27: MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:315]
#28: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:291]
#29: XRE_InitChildProcess(int, char**, XREChildData const*) [toolkit/xre/nsEmbedFunctions.cpp:781]
#30: content_process_main(mozilla::Bootstrap*, int, char**) [ipc/contentproc/plugin-container.cpp:57]
#31: main [browser/app/nsBrowserApp.cpp:303]
#32: libc.so.6 + 0x271e3

(In reply to Bob Clary [:bc:] from comment #0)
This is more likely to be a wrapper cache issue.

Andrew, do you know who would be good to look into this?

Flags: needinfo?(continuation)
Group: core-security → javascript-core-security

I can try taking a look, if I can figure out how to use the squid proxy.

I think the assertion is saying that the reflector that we have in the wrapper cache is not the same as the object that is pointing at the wrapper cached object.

The test URLs seem to be complete junk, which is strange. Maybe it is one of those cases where we fail in the middle of setting up the page. I have no idea how a proxy would play into that in a way that would affect the DOM.

Assignee: nobody → continuation
Group: javascript-core-security → dom-core-security
Component: JavaScript: GC → DOM: Core & HTML
Flags: needinfo?(continuation)

I haven't had a ton of time to look into this yet, but I did manage to reproduce it. I poked at it a bit in GDB, and found that the C++ type for the DOM object being wrapped is nsGlobalWindowInner, and the assertion is failing because mWrapper is null.

Yeah, the real question is how mWrapper got to be null before it got finalized, while still pointing at our object.

It looks like the call to CreateGlobalOptions<T>::PostCreateGlobal(aCx, aGlobal) is failing, because I see an NS_ENSURE_SUCCESS failure on the result of this line:
nsresult rv = BasePrincipal::Cast(principal)->GetSiteIdentifier(site);
from the function CreateGlobalOptionsWithXPConnect::PostCreateGlobal. Presumably that is due to some principal code choking on the bizarre URL (though I don't know why this would only happen with the proxy).

This happens in CreateGlobal shortly after js::SetReservedSlot(aGlobal, DOM_OBJECT_SLOT, JS::PrivateValue(aNative)); and aCache->SetWrapper(aGlobal); happen.

When CreateGlobal fails, we end up back in the failure case in the Wrap() method in WindowBinding.cpp. The error handling code in there calls ReleaseWrapper() and ClearWrapper() on the cache, so we end up with mWrapper as null, but as far as I can see nothing attempts to clear the pointer from the reflector to the inner window, so later when the reflector gets destroyed, we hit the assertion.

There's a class BindingJSObjectCreator that seems to deal with clearing DOM_OBJECT_SLOT in case of failure, but I don't see it used for inner windows.

I tried setting DOM_OBJECT_SLOT to undefined and calling release on aObject (in Wrap) but then I hit an assertion in ~nsGlobalWindowInner() MOZ_ASSERT(!mHintedWasLoading), which makes sense because we've bailed out in the middle of loading. I tried to call HintIsLoading(false) to clear the flag, but that causes some other kind of crash.

I'm not sure if there are any real bad security things that could happen as a result of it. I would think that neither the window nor the reflector would escape to where anything can access them, and even if they did, I wouldn't think you'd get anything worse than a null deref.

This looks like improper error handling in code generated by WebIDL bindings, so I'll move it to that component.

Component: DOM: Core & HTML → DOM: Bindings (WebIDL)

Any thoughts, Boris?

Flags: needinfo?(bzbarsky)

Thank you for digging into this!

GetSiteIdentifier failing is bad and we should look into not doing that... Could use a separate bug.

Yes, BindingJSObjectCreator is not used for globals. Maybe we should fix that (and move dom::CreateGlobal to be yet another, templated, method on BindingJSObjectCreator). But the failing GetSiteIdentifier is still a problem in terms of doing something the user actually wants, like loading the page.

but then I hit an assertion in ~nsGlobalWindowInner(), MOZ_ASSERT(!mHintedWasLoading), which makes sense because we've bailed out in the middle of loading

Right, because we call SetActiveLoadingState before calling CreateNativeGlobalForInner in nsGlobalWindowOuter::SetNewDocument. But note that we then assert that CreateNativeGlobalForInner succeeded... We're really not set up well to handle failures in that code. :(

Flags: needinfo?(bzbarsky)

For posterity, the steps I used to get Squid working on Linux were:

  1. Install squid. I think (?) I kept the default settings.
  2. Start squid running with sudo systemctl start squid.service.
  3. Start Firefox. In Settings, search for "proxy". Select "manual proxy configuration" and under "HTTP proxy" set 127.0.0.1 and port 3128.
  4. paste .google.hu in the URL bar and hit enter to load the page.

Can I unhide this, Boris?

(I'm also working on figuring out enough of what is going on that I can file a bug about GetSiteIdentifier failing.)

Flags: needinfo?(bzbarsky)

On the bindings side, yes.

I don't know whether there are security implications of GetSiteIdentifier failing...

Flags: needinfo?(bzbarsky)

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

I don't know whether there are security implications of GetSiteIdentifier failing...

As far as I can see, GetSideIdentifier() fails because nsEffectiveTLDService::GetBaseDomainInternal() realizes that the hostname begins with a . and bails: https://searchfox.org/mozilla-central/rev/a1592902acabf9bded973067133baaac1457f3d3/netwerk/dns/nsEffectiveTLDService.cpp#299

I don't know how that would have security implications, but I'm not familiar with any of this TLD stuff.

In general for this bug, I assume that the basic issue is that in the non-proxy case networking code ends up running GetSiteIdentifier() on it earlier before we decide to go ahead and create the window, but in the proxy case it defers to the proxy, so we do end up going ahead and creating a window that we can't possibly load. I don't know precisely where that is happening, but there's some kind of related code in nsSocketTransport::ResolveHost() where it bails out before doing DNS. There's even a little bit of filtering (added in bug 304904 as some kind of security measure against tricking proxies) where it only allows certain characters in the host name. In fact, if I alter that function, net_IsValidHostName, to return false if host is empty or starts with ".", then we avoid the crash entirely, but I don't know if that's a general enough fix or if there are other hostnames that GetSideIdentifier will reject that net_IsValidHostName allows.

Ehsan, should we bw dong some sort of check earlier, before we create the document?

Flags: needinfo?(ehsan)

Yes if I understand the bug correctly it seems to me that we should be doing some sort of check earlier. However it's not obvious to me if the right fix is to modify net_IsValidHostName. That function is used for fetches that aren't navigations, and from what I understand it is only for navigations which we do not support a leading dot in the host name, right?

I wonder if Honza has some input here too.

Flags: needinfo?(ehsan) → needinfo?(honzab.moz)
Flags: needinfo?(honzab.moz)

(In reply to :ehsan akhgari from comment #17)

Yes if I understand the bug correctly it seems to me that we should be doing some sort of check earlier. However it's not obvious to me if the right fix is to modify net_IsValidHostName. That function is used for fetches that aren't navigations, and from what I understand it is only for navigations which we do not support a leading dot in the host name, right?

The URLs are not valid, as they contain empty labels, so we should probably reject them anyway. Changing net_IsValidHostName seems right.
I see Chrome turns http://.google.hu/ into http://google.hu/ - we might want to fixup the URL to do that too, and it would happen only for navigations, which is good.

Group: dom-core-security

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Keywords: regression
Severity: normal → S3
Assignee: continuation → nobody
You need to log in before you can comment on or make changes to this bug.