This bug was filed from the Socorro interface and is report bp-f043bb0d-5081-4421-8b23-e01630180704. ============================================================= Top 10 frames of crashing thread: 0 mozglue.dll MOZ_CrashPrintf mfbt/Assertions.cpp:63 1 xul.dll InvalidArrayIndex_CRASH xpcom/ds/nsTArray.cpp:26 2 xul.dll nsTArray_Impl<Expr*, nsTArrayInfallibleAllocator>::operator[] xpcom/ds/nsTArray.h:1069 3 xul.dll nsWebBrowserPersist::SerializeNextFile dom/webbrowserpersist/nsWebBrowserPersist.cpp:612 4 xul.dll mozilla::detail::RunnableMethodImpl<RefPtr<mozilla::SourceListener>, void xpcom/threads/nsThreadUtils.h:1217 5 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1051 6 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:97 7 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:318 8 xul.dll MessageLoop::Run ipc/chromium/src/base/message_loop.cc:298 9 xul.dll nsBaseAppShell::Run widget/nsBaseAppShell.cpp:158 ============================================================= There are 10 crashes (from 8 installations) in nightly 63 starting with buildid 20180704100142. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1469916. [1] https://hg.mozilla.org/mozilla-central/rev?node=9a2b02fe351b

So clearly we can't assume that the doclist is non-empty, though as I commented in the code here, I don't understand how this can be the case while there are still more files to serialize. Then again, the code isn't/wasn't the clearest to begin with - there are several comments that say "we must bail", and they proceed to "only" break out of the loop. Solid STR would really help. Just staring at the code, maybe something like this could happen: 1. we start serializing a document 2. the very first subresource fails to save for some reason, so we bail out of the mURIMap loop (which saves the subresources) here: https://dxr.mozilla.org/mozilla-central/rev/6c0fa9a675c91390ca27664ffb626c56e8afea4d/dom/webbrowserpersist/nsWebBrowserPersist.cpp#638-647 3. so then we end up saving the document here: https://dxr.mozilla.org/mozilla-central/rev/6c0fa9a675c91390ca27664ffb626c56e8afea4d/dom/webbrowserpersist/nsWebBrowserPersist.cpp#665-700 4. and end up calling OnWrite::OnFinish when we're done, which queues the task which the event loop is processing to call SerializeNextFile again (AFAICT, only to make sure we clean up and call FinishDownload), and then we blow up because nothing ever cleared mURIMap. (Note that in theory, the webbrowserpersist code pretends to support serializing multiple documents per instance, but (a) I don't think any consumers try to do this, and (b) I would be really scared if they did... - I don't see any reason it'd reliably work.) Christoph, can you check if my suggestion about what's up makes sense to you? (I don't suppose you have any ideas about other people who actually know this code? :) )

(In reply to :Gijs (he/him) from comment #1) > (Note that in theory, the webbrowserpersist code pretends to support > serializing multiple documents per instance, but (a) I don't think any > consumers try to do this, and (b) I would be really scared if they did... - > I don't see any reason it'd reliably work.) OK, so I think this is true - but it definitely actually tries to recursively save subframes, too, and I could see how that'd cause the current code to be wrong anyway. I guess to fix the whole thing we should probably store the principal with the uri data instead of trying to figure out which document the URI data belonged to. Though this all also affects bug 332676 and friends...

Comment on attachment 8990359 [details] Bug 1473509 - store principal information with the URIs to avoid having to locate documents after the fact, https://reviewboard.mozilla.org/r/255420/#review262246 ::: dom/webbrowserpersist/nsWebBrowserPersist.cpp (Diff revision 1) > struct nsWebBrowserPersist::DocData > { > nsCOMPtr<nsIURI> mBaseURI; > nsCOMPtr<nsIWebBrowserPersistDocument> mDocument; > nsCOMPtr<nsIURI> mFile; > - nsCOMPtr<nsIPrincipal> mPrincipal; This seems to just be an oversight that I forgot to remove when landing bug 1469916. It's unused. Not sure why the compiler doesn't warn for this.

Note that the duplicate bug has STR, which I verified no longer crash with this patch.

Comment on attachment 8990359 [details] Bug 1473509 - store principal information with the URIs to avoid having to locate documents after the fact, https://reviewboard.mozilla.org/r/255420/#review262272

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/d8e6b69bc535 store principal information with the URIs to avoid having to locate documents after the fact, r=mccr8