Closed Bug 1247866 Opened 8 years ago Closed 8 years ago

Attempting to save hellocasa.fr causes fatal "Assertion failure: IsIdle(oldState), at PLDHashTable.h:13", several layers inside of nsWebBrowserPersist::FixRedirectedChannelEntry

Categories

(Core Graveyard :: Embedding: APIs, defect)

defect
Not set
normal

Tracking

(firefox47 fixed)

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

Details

(Keywords: assertion)

Attachments

(3 files)

STR:
 1. In a debug build, visit https://hellocasa.fr
      (Ignore assertions about pseudotype)

 2. File | Save
 3. Type in some place to save it, e.g. /tmp/f.html

ACTUAL RESULTS:
Before the save operation is complete, I hit a fatal assertion-failure:
{
Assertion failure: IsIdle(oldState), at ../../../mozilla/xpcom/glue/PLDHashTable.h:132
#01: Checker::StartWriteOp() (/scratch/work/builds/mozilla-inbound/obj/xpcom/build/../../dist/include/PLDHashTable.h:132 (discriminator 2))
#02: AutoWriteOp (/scratch/work/builds/mozilla-inbound/mozilla/xpcom/glue/PLDHashTable.cpp:35)
#03: PLDHashTable::Remove(void const*) (/scratch/work/builds/mozilla-inbound/mozilla/xpcom/glue/PLDHashTable.cpp:619)
#04: nsTHashtable<nsBaseHashtableET<nsISupportsHashKey, nsAutoPtr<nsWebBrowserPersist::OutputData> > >::RemoveEntry(nsISupports*) (/scratch/work/builds/mozilla-inbound/obj/embedding/components/webbrowserpersist/../../../dist/include/nsTHashtable.h:170)
#05: nsBaseHashtable<nsISupportsHashKey, nsAutoPtr<nsWebBrowserPersist::OutputData>, nsWebBrowserPersist::OutputData*>::Remove(nsISupports*) (/scratch/work/builds/mozilla-inbound/obj/embedding/components/webbrowserpersist/../../../dist/include/nsBaseHashtable.h:147)
#06: nsClassHashtable<nsISupportsHashKey, nsWebBrowserPersist::OutputData>::RemoveAndForget(nsISupports*, nsAutoPtr<nsWebBrowserPersist::OutputData>&) (/scratch/work/builds/mozilla-inbound/obj/embedding/components/webbrowserpersist/../../../dist/include/nsClassHashtable.h:134 (discriminator 1))
#07: nsWebBrowserPersist::FixRedirectedChannelEntry(nsIChannel*) (/scratch/work/builds/mozilla-inbound/obj/embedding/components/webbrowserpersist/../../../../mozilla/embedding/components/webbrowserpersist/nsWebBrowserPersist.cpp:2408)
#08: nsWebBrowserPersist::OnStartRequest(nsIRequest*, nsISupports*) (/scratch/work/builds/mozilla-inbound/obj/embedding/components/webbrowserpersist/../../../../mozilla/embedding/components/webbrowserpersist/nsWebBrowserPersist.cpp:825)
#09: non-virtual thunk to nsWebBrowserPersist::OnStartRequest(nsIRequest*, nsISupports*) (/scratch/work/builds/mozilla-inbound/obj/embedding/components/webbrowserpersist/../../../../mozilla/embedding/components/webbrowserpersist/nsWebBrowserPersist.cpp:796)
#10: mozilla::net::nsHttpChannel::CallOnStartRequest() (/scratch/work/builds/mozilla-inbound/mozilla/netwerk/protocol/http/nsHttpChannel.cpp:993)
#11: mozilla::net::nsHttpChannel::ContinueOnStartRequest3(nsresult) (/scratch/work/builds/mozilla-inbound/mozilla/netwerk/protocol/http/nsHttpChannel.cpp:5884)
#12: mozilla::net::nsHttpChannel::ContinueOnStartRequest2(nsresult) (/scratch/work/builds/mozilla-inbound/mozilla/netwerk/protocol/http/nsHttpChannel.cpp:5875)
#13: mozilla::net::nsHttpChannel::ContinueOnStartRequest1(nsresult) (/scratch/work/builds/mozilla-inbound/mozilla/netwerk/protocol/http/nsHttpChannel.cpp:5852)
#14: mozilla::net::nsHttpChannel::OnStartRequest(nsIRequest*, nsISupports*) (/scratch/work/builds/mozilla-inbound/mozilla/netwerk/protocol/http/nsHttpChannel.cpp:5824)
#15: non-virtual thunk to mozilla::net::nsHttpChannel::OnStartRequest(nsIRequest*, nsISupports*) (/scratch/work/builds/mozilla-inbound/mozilla/netwerk/protocol/http/nsHttpChannel.cpp:5743)
#16: nsInputStreamPump::OnStateStart() (/scratch/work/builds/mozilla-inbound/mozilla/netwerk/base/nsInputStreamPump.cpp:525)
#17: nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (/scratch/work/builds/mozilla-inbound/mozilla/netwerk/base/nsInputStreamPump.cpp:427)
#18: non-virtual thunk to nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (/scratch/work/builds/mozilla-inbound/mozilla/netwerk/base/nsInputStreamPump.cpp:393)
#19: nsInputStreamReadyEvent::Run() (/scratch/work/builds/mozilla-inbound/mozilla/xpcom/io/nsStreamUtils.cpp:94)
#20: nsThread::ProcessNextEvent(bool, bool*) (/scratch/work/builds/mozilla-inbound/mozilla/xpcom/threads/nsThread.cpp:1019)
#21: NS_ProcessNextEvent(nsIThread*, bool) (/scratch/work/builds/mozilla-inbound/mozilla/xpcom/glue/nsThreadUtils.cpp:297)
#22: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/scratch/work/builds/mozilla-inbound/mozilla/ipc/glue/MessagePump.cpp:95)
#23: MessageLoop::RunInternal() (/scratch/work/builds/mozilla-inbound/mozilla/ipc/chromium/src/base/message_loop.cc:235)
#24: MessageLoop::RunHandler() (/scratch/work/builds/mozilla-inbound/mozilla/ipc/chromium/src/base/message_loop.cc:228)
#25: MessageLoop::Run() (/scratch/work/builds/mozilla-inbound/mozilla/ipc/chromium/src/base/message_loop.cc:201)
#26: nsBaseAppShell::Run() (/scratch/work/builds/mozilla-inbound/mozilla/widget/nsBaseAppShell.cpp:156)
#27: nsAppStartup::Run() (/scratch/work/builds/mozilla-inbound/mozilla/toolkit/components/startup/nsAppStartup.cpp:281)
#28: XREMain::XRE_mainRun() (/scratch/work/builds/mozilla-inbound/obj/toolkit/xre/../../../mozilla/toolkit/xre/nsAppRunner.cpp:4283)
#29: XREMain::XRE_main(int, char**, nsXREAppData const*) (/scratch/work/builds/mozilla-inbound/obj/toolkit/xre/../../../mozilla/toolkit/xre/nsAppRunner.cpp:4380)
#30: XRE_main (/scratch/work/builds/mozilla-inbound/obj/toolkit/xre/../../../mozilla/toolkit/xre/nsAppRunner.cpp:4482)
#31: do_main(int, char**, char**, nsIFile*) (/scratch/work/builds/mozilla-inbound/obj/browser/app/../../../mozilla/browser/app/nsBrowserApp.cpp:220)
#32: main (/scratch/work/builds/mozilla-inbound/obj/browser/app/../../../mozilla/browser/app/nsBrowserApp.cpp:360)
#33: __libc_start_main (/build/buildd/glibc-2.21/csu/libc-start.c:323)
#34: _start (./dist/bin/firefox)
#35: ??? (???:???)
}
In case it's easier, here's the assertion backtrace as a text file attachment.
Tested with e10s enabled & disabled -- the assertion/abort happens in both cases.
(Nothing obviously goes wrong in a Nightly build, though.)
Note that the process that's aborting here is the *chrome process* ([Parent]), which kinda makes sense because that's the process which does the saving presumably.

In nsWebBrowserPersist::FixRedirectedChannelEntry, we have the following:
> 2394     for (auto iter = mOutputMap.Iter(); !iter.Done(); iter.Next()) {
> 2395         nsISupports* key = iter.Key();
[...]
> 2404         if (matchingURI) {
[...]
> 2408             mOutputMap.RemoveAndForget(key, outputData);
[...]
> 2417             break;
> 2418         }
http://mxr.mozilla.org/mozilla-central/source/embedding/components/webbrowserpersist/nsWebBrowserPersist.cpp#2394

So, we're calling RemoveAndForget on the hashtable, while we're iterating over it. This looks potentially-sketchy (I'm assuming we shouldn't be modifying the hashtable, except via the iterator's own .Remove() API, while we iterate over it).

I think it actually turns out to be probably-safe in practice, because we break out of the loop right after we modify the hashtable.  But I'll bet the fatal assertion is warning us that what we're doing is potentially unsafe [if we didn't have the break statement].
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Here's a patch. This just moves some code.

(We could use the iter.Remove() API here instead, probably, but that would require a bit more rewriting.  Since we're stopping iteration at the point of removal anyway, seems simplest to just keep using the standard not-safe-during-iteration removal method, and just postpone it to happen after we've broken out of the loop.)
Attachment #8718964 - Flags: review?(n.nethercote)
Here's a no-whitespace-changes version of the patch, which perhaps shows the code-move a bit more clearly.
Attachment #8718984 - Attachment is patch: true
Attachment #8718984 - Attachment mime type: text/x-patch → text/plain
I've verified that the patch prevents us from hitting this fatal assertion.

I'm pretty sure that means this bug doesn't need to be security-sensitive, per end of comment 4. njn, please sanity-check me on that, though...

For what it's worth, when we trip this assertion, the hash table's "oldState" is set to "kRead1" -- and I'm pretty sure it's set that way because we're iterating [though about to stop iterating].
Keywords: sec-audit
Comment on attachment 8718964 [details] [diff] [review]
fix v1: Postpone removal slightly (do it just after exiting loop, not just before)

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

Looks good. Thank you for the fix.

I agree that the old code wasn't a problem in practice and that the s-s status can be removed.

::: embedding/components/webbrowserpersist/nsWebBrowserPersist.cpp
@@ +2414,5 @@
> +        nsAutoPtr<OutputData> outputData;
> +        mOutputMap.RemoveAndForget(matchingKey, outputData);
> +        NS_ENSURE_TRUE(outputData, NS_ERROR_FAILURE);
> +
> +        // Store data again with new channel unless told to ignore redirects

While you're here please add a period to the end of the sentence.
Attachment #8718964 - Flags: review?(n.nethercote) → review+
Group: core-security
Thanks! I'll tweak that comment as suggested before landing.
Keywords: sec-audit
https://hg.mozilla.org/mozilla-central/rev/3537591e1de1
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: