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)
Core Graveyard
Embedding: APIs
Tracking
(firefox47 fixed)
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: dholbert, Assigned: dholbert)
Details
(Keywords: assertion)
Attachments
(3 files)
5.17 KB,
text/plain
|
Details | |
3.11 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
2.12 KB,
patch
|
Details | Diff | Splinter Review |
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: ??? (???:???) }
Assignee | ||
Comment 1•8 years ago
|
||
In case it's easier, here's the assertion backtrace as a text file attachment.
Assignee | ||
Comment 2•8 years ago
|
||
Tested with e10s enabled & disabled -- the assertion/abort happens in both cases.
Assignee | ||
Comment 3•8 years ago
|
||
(Nothing obviously goes wrong in a Nightly build, though.)
Assignee | ||
Comment 4•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•8 years ago
|
||
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)
Assignee | ||
Comment 6•8 years ago
|
||
Here's a no-whitespace-changes version of the patch, which perhaps shows the code-move a bit more clearly.
Assignee | ||
Updated•8 years ago
|
Attachment #8718984 -
Attachment is patch: true
Attachment #8718984 -
Attachment mime type: text/x-patch → text/plain
Assignee | ||
Comment 7•8 years ago
|
||
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].
Comment 8•8 years ago
|
||
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+
Updated•8 years ago
|
Group: core-security
Assignee | ||
Comment 9•8 years ago
|
||
Thanks! I'll tweak that comment as suggested before landing.
Keywords: sec-audit
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3537591e1de1
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Updated•5 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•