Details

Reviewers

tnikkel
longsonr

Commits

rMOZILLACENTRAL50b298a5272a: Bug 1624118 - Fix various issues with view handling when loading <object> /…

Bugzilla Bug ID

1624118

Summary

These look like two really old bugs.

Part of the issue is that <object> / <embed> manage their frame loader quite
differently from <iframe>. This means that we may have a PresShell /
ViewManager / etc for a frame loader that doesn't yet have a frame associated.

For example, this is the viewport creation for the SVG document that reproduces
the problem:

#0 0x00005cc60e8302e7 in mozilla::ViewportFrame::SetViewInternal(nsView*) (this=0x68599020, aView=0x683d8f00) at /home/emilio/src/moz/gecko/obj-debug/dist/include/mozilla/ViewportFrame.h:106
#1 0x00005cc60e842858 in nsIFrame::SetView(nsView*) (this=0x68599020, aView=0x683d8f00) at /home/emilio/src/moz/gecko/layout/generic/nsFrame.cpp:7057
#2 0x00005cc60e77258a in nsCSSFrameConstructor::ConstructRootFrame() (this=0xc72c715e00) at /home/emilio/src/moz/gecko/layout/base/nsCSSFrameConstructor.cpp:2424
#3 0x00005cc60e7342f5 in mozilla::PresShell::Initialize() (this=0x6830e000) at /home/emilio/src/moz/gecko/layout/base/PresShell.cpp:1758
#4 0x00005cc60c9fb02a in nsContentSink::StartLayout(bool) (this=<optimized out>, aIgnorePendingSheets=<optimized out>) at /home/emilio/src/moz/gecko/dom/base/nsContentSink.cpp:1160
#5 0x00005cc60e2c1581 in nsXMLContentSink::HandleStartElement(char16_t const*, char16_t const**, unsigned int, unsigned int, unsigned int, bool)

	    (this=<optimized out>, aName=<optimized out>, aAtts=0x6fde8200, aAttsCount=<optimized out>, aLineNumber=3, aColumnNumber=<optimized out>, aInterruptable=true) at /home/emilio/src/moz/gecko/dom/xml/nsXMLContentSink.cpp:982

#6 0x00005cc60e2c17d7 in non-virtual thunk to nsXMLContentSink::HandleStartElement(char16_t const*, char16_t const, unsigned int, unsigned int, unsigned int) () at /home/emilio/src/moz/gecko/dom/xml/nsXMLContentSink.cpp:889
#7 0x00005cc60c360307 in nsExpatDriver::HandleStartElement(void*, char16_t const*, char16_t const) (aUserData=0x224f650d0cc0, aName=0x685aef20 u"http://www.w3.org/2000/svg\xffffdesc", aAtts=0x6fde8200)

	    at /home/emilio/src/moz/gecko/parser/htmlparser/nsExpatDriver.cpp:293

#8 0x00005cc60ee91cea in doContent (parser=0xc72c70f800, startTagLevel=0, enc=<optimized out>, s=<optimized out>, end=0x5bbd31af5020 "\344\344\344", <incomplete sequence \344>, nextPtr=0x7ffca2653288, haveMore=1 '\001')

	    at /home/emilio/src/moz/gecko/parser/expat/lib/xmlparse.c:2872

#9 0x00005cc60ee90059 in contentProcessor (parser=0xc72c70f800, start=0xffffffe6 <error: Cannot access memory at address 0xffffffe6>, end=0xc72c511360 "", endPtr=0x1) at /home/emilio/src/moz/gecko/parser/expat/lib/xmlparse.c:2528
#10 0x00005cc60ee8f8d5 in doProlog

	    (parser=<optimized out>, enc=0x5cc612ce0930 <little2_encoding_ns>, s=0x5bbd31ab508e "<", end=0x5bbd31af5020 "\344\344\344", <incomplete sequence \344>, tok=<optimized out>, next=<optimized out>, nextPtr=0x7ffca2653288, haveMore=1 '\001', allowClosingDoctype=1 '\001') at /home/emilio/src/moz/gecko/parser/expat/lib/xmlparse.c:4591

#11 0x00005cc60ee8d86e in prologProcessor (parser=0xc72c70f800, s=0x5bbd31ab5020 "<", end=0x5bbd31af5020 "\344\344\344", <incomplete sequence \344>, nextPtr=0x7ffca2653288) at /home/emilio/src/moz/gecko/parser/expat/lib/xmlparse.c:4311
#12 0x00005cc60ee8cf45 in MOZ_XML_Parse (parser=0xc72c70f800, s=0x5bbd31ab5020 "<", len=262144, isFinal=0) at /home/emilio/src/moz/gecko/parser/expat/lib/xmlparse.c:1894
#13 0x00005cc60c3627bc in nsExpatDriver::ParseBuffer(char16_t const*, unsigned int, bool, unsigned int*)

	    (this=0x224f650d0cc0, aBuffer=0x5bbd31ab5020 u"<?xml version=\"1.0\" encoding=\"UTF-8\" standalone=\"no\"?>\n<svg height=\"2970\" width=\"2100\" viewBox=\"0 0 2100 2970\" version=\"1.1\" xmlns=\"http://www.w3.org/2000/svg\" xmlns:xlink=\"http://www.w3.org/1999/xlin"..., aLength=131072, aIsFinal=false, aConsumed=<optimized out>) at /home/emilio/src/moz/gecko/parser/htmlparser/nsExpatDriver.cpp:875

#14 0x00005cc60c362c91 in nsExpatDriver::ConsumeToken(nsScanner&, bool&) (this=<optimized out>, aScanner=..., aFlushTokens=<optimized out>) at /home/emilio/src/moz/gecko/parser/htmlparser/nsExpatDriver.cpp:970
#15 0x00005cc60c3666a8 in nsParser::Tokenize(bool) (this=0x224f65038e80, aIsFinalChunk=false) at /home/emilio/src/moz/gecko/parser/htmlparser/nsParser.cpp:1410
#16 0x00005cc60c36595e in nsParser::ResumeParse(bool, bool, bool) (this=0x224f65038e80, allowIteration=true, aIsFinalChunk=false, aCanInterrupt=<optimized out>) at /home/emilio/src/moz/gecko/parser/htmlparser/nsParser.cpp:961
#17 0x00005cc60c366c86 in nsParser::OnDataAvailable(nsIRequest*, nsIInputStream*, unsigned long, unsigned int) (this=0x224f65038e80, request=<optimized out>, pIStream=0x6fdfc430, sourceOffset=<optimized out>, aLength=131072)

	    at /home/emilio/src/moz/gecko/parser/htmlparser/nsParser.cpp:1317

#18 0x00005cc60c897cc2 in nsObjectLoadingContent::OnDataAvailable(nsIRequest*, nsIInputStream*, unsigned long, unsigned int) (this=<optimized out>, aRequest=0x68483080, aInputStream=0x6fdfc430, aOffset=0, aCount=131072)

	    at /home/emilio/src/moz/gecko/dom/base/nsObjectLoadingContent.cpp:1055

#19 0x00005cc60b9d18f8 in mozilla::net::HttpChannelChild::DoOnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int)

	    (this=0x68483000, aRequest=0x68483080, aContext=<optimized out>, aStream=0x6fdfc430, aOffset=0, aCount=743510880) at /home/emilio/src/moz/gecko/netwerk/protocol/http/HttpChannelChild.cpp:968

#20 0x00005cc60b9d5cbf in mozilla::net::HttpChannelChild::OnTransportAndData(nsresult const&, nsresult const&, unsigned long const&, unsigned int const&, nsTString<char> const&)

	    (this=0x68483000, aChannelStatus=<optimized out>, aTransportStatus=@0x683be5bc: -2142568440, aOffset=<optimized out>, aCount=<optimized out>, aData=...) at /home/emilio/src/moz/gecko/netwerk/protocol/http/HttpChannelChild.cpp:867

#21 0x00005cc60badb535 in mozilla::net::ChannelEventQueue::FlushQueue() (this=0xc72ce2cae0) at /home/emilio/src/moz/gecko/netwerk/ipc/ChannelEventQueue.cpp:90
#22 0x00005cc60b976fda in mozilla::net::ChannelEventQueue::MaybeFlushQueue() (this=0xc72ce2cae0) at /home/emilio/src/moz/gecko/obj-debug/dist/include/mozilla/net/ChannelEventQueue.h:350
#23 0x00005cc60baec442 in mozilla::net::ChannelEventQueue::CompleteResume() (this=0xc72ce2cae0) at /home/emilio/src/moz/gecko/obj-debug/dist/include/mozilla/net/ChannelEventQueue.h:329
#24 mozilla::net::ChannelEventQueue::ResumeInternal()::CompleteResumeRunnable::Run() (this=<optimized out>) at /home/emilio/src/moz/gecko/netwerk/ipc/ChannelEventQueue.cpp:148
#25 0x00005cc60b53d4f1 in mozilla::SchedulerGroup::Runnable::Run() (this=0x685b0200) at /home/emilio/src/moz/gecko/xpcom/threads/SchedulerGroup.cpp:282
#26 0x00005cc60b54ff80 in nsThread::ProcessNextEvent(bool, bool*) (this=0x3dd7f4f3020, aMayWait=<optimized out>, aResult=0x7ffca2653ea7) at /home/emilio/src/moz/gecko/xpcom/threads/nsThread.cpp:1220
#27 0x00005cc60b552f0d in NS_ProcessNextEvent(nsIThread*, bool) (aThread=0x68599020, aMayWait=true) at /home/emilio/src/moz/gecko/xpcom/threads/nsThreadUtils.cpp:481

The parent view may not be set properly if the frame is not current by the time
it is created. For example in this case the parent for the root view is
non-null and comes from the following MakeWindow call:

#0 nsDocumentViewer::MakeWindow(nsSize const&, nsView*) (this=0xc72ca72cd0, aSize=..., aContainerView=0x683ba500) at /home/emilio/src/moz/gecko/layout/base/nsDocumentViewer.cpp:2368
#1 0x00005cc60e789b50 in nsDocumentViewer::InitInternal(nsIWidget*, nsISupports*, mozilla::dom::WindowGlobalChild*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, bool, bool, bool)

	    (this=0xc72ca72cd0, aParentWidget=<optimized out>, aState=0x0, aActor=0x0, aBounds=..., aDoCreation=true, aNeedMakeCX=<optimized out>, aForceSetNewDocument=<optimized out>)
	    at /home/emilio/src/moz/gecko/layout/base/nsDocumentViewer.cpp:933

#2 0x00005cc60e789959 in nsDocumentViewer::Init(nsIWidget*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::dom::WindowGlobalChild*)

	    (this=0xc72ca72cd0, aParentWidget=0x7ffca2651020, aBounds=..., aActor=0x7f6216725c00) at /home/emilio/src/moz/gecko/layout/base/nsDocumentViewer.cpp:762

#3 0x00005cc60f4f584f in nsDocShell::SetupNewViewer(nsIContentViewer*, mozilla::dom::WindowGlobalChild*) (this=0x684db000, aNewViewer=<optimized out>, aWindowActor=<optimized out>)

	    at /home/emilio/src/moz/gecko/docshell/base/nsDocShell.cpp:8017

#4 0x00005cc60f4f5204 in nsDocShell::Embed(nsIContentViewer*, mozilla::dom::WindowGlobalChild*) (this=0x684db000, aContentViewer=0x7ffca2651020, aWindowActor=0x683ba500) at /home/emilio/src/moz/gecko/docshell/base/nsDocShell.cpp:5745
#5 0x00005cc60f4dbc7b in nsDocShell::CreateContentViewer(nsTSubstring<char> const&, nsIRequest*, nsIStreamListener**) (this=0x684db000, aContentType=..., aRequest=0x68483080, aContentHandler=<optimized out>)

	    at /home/emilio/src/moz/gecko/docshell/base/nsDocShell.cpp:7819

#6 0x00005cc60f4dab99 in nsDSURIContentListener::DoContent(nsTSubstring<char> const&, bool, nsIRequest*, nsIStreamListener**, bool*)

	    (this=0x683056a0, aContentType=..., aIsContentPreferred=<optimized out>, aRequest=0x68483080, aContentHandler=0xc72c5e8608, aAbortProcess=0x7ffca265139f) at /home/emilio/src/moz/gecko/docshell/base/nsDSURIContentListener.cpp:181

#7 0x00005cc60c2fd8f5 in nsDocumentOpenInfo::TryContentListener(nsIURIContentListener*, nsIChannel*) (this=0xc72c5e85e0, aListener=0x683056a0, aChannel=<optimized out>) at /home/emilio/src/moz/gecko/uriloader/base/nsURILoader.cpp:632
#8 0x00005cc60c2fccd1 in nsDocumentOpenInfo::DispatchContent(nsIRequest*, nsISupports*) (this=0xc72c5e85e0, request=0x68483080, aCtxt=<optimized out>) at /home/emilio/src/moz/gecko/uriloader/base/nsURILoader.cpp:313
#9 0x00005cc60c2fc5aa in nsDocumentOpenInfo::OnStartRequest(nsIRequest*) (this=<optimized out>, request=0x68483080) at /home/emilio/src/moz/gecko/uriloader/base/nsURILoader.cpp:191
#10 0x00005cc60c8975c4 in nsObjectLoadingContent::LoadObject(bool, bool, nsIRequest*) (this=0x4b1b3938b6a8, aNotify=<optimized out>, aForceLoad=<optimized out>, aLoadingChannel=0x68483080)

	    at /home/emilio/src/moz/gecko/dom/base/nsObjectLoadingContent.cpp:2218

#11 0x00005cc60c89681f in nsObjectLoadingContent::OnStartRequest(nsIRequest*) (this=0x4b1b3938b6a8, aRequest=0x68483080) at /home/emilio/src/moz/gecko/dom/base/nsObjectLoadingContent.cpp:1006
#12 0x00005cc60b9d1020 in mozilla::net::HttpChannelChild::DoOnStartRequest(nsIRequest*, nsISupports*) (this=0x68483000, aRequest=0x68483080, aContext=<optimized out>)

	    at /home/emilio/src/moz/gecko/netwerk/protocol/http/HttpChannelChild.cpp:708

#13 0x00005cc60b9d481b in mozilla::net::HttpChannelChild::OnStartRequest(nsresult const&, mozilla::net::nsHttpResponseHead const&, bool const&, mozilla::net::nsHttpHeaderArray const&, mozilla::net::ParentLoadInfoForwarderArgs const&, bool const&, bool const&, bool const&, unsigned long const&, int const&, unsigned int const&, nsTString<char> const&, nsTString<char> const&, mozilla::net::NetAddr const&, mozilla::net::NetAddr const&, unsigned int const&, nsTString<char> const&, long const&, bool const&, bool const&, bool const&, mozilla::net::ResourceTimingStructArgs const&, bool const&, mozilla::Maybe<unsigned int> const&, bool const&, nsILoadInfo::CrossOriginOpenerPolicy const&)

However, even though aContainerView is non-null, the view is incorrect, it's
the view for the _old_ frame.

The view parent/child relationship gets cleared properly in:

#1 0x00005cc60e8e82bf in BeginSwapDocShellsForViews (aSibling=0x0) at /home/emilio/src/moz/gecko/layout/generic/nsSubDocumentFrame.cpp:1027
warning: Source file is more recent than executable.
#2 0x00005cc60e8e810b in nsSubDocumentFrame::DestroyFrom (this=0x6cd04eaa45a8, aDestructRoot=0x6cd04eaa45a8, aPostDestroyData=...) at /home/emilio/src/moz/gecko/layout/generic/nsSubDocumentFrame.cpp:943
#3 0x00005cc60e7b71a3 in nsIFrame::Destroy (this=0x6cd04eaa45a8) at /home/emilio/src/moz/gecko/layout/generic/nsIFrame.h:657
#4 0x00005cc60e80baac in nsBlockFrame::RemoveFrame (this=0x4b1b39362d88, aListID=<optimized out>, aOldFrame=0x6cd04eaa45a8) at /home/emilio/src/moz/gecko/layout/generic/nsBlockFrame.cpp:5597
#5 0x00005cc60e8df29f in nsPlaceholderFrame::DestroyFrom (this=0x4b1b39363240, aDestructRoot=0x4b1b39363240, aPostDestroyData=...) at /home/emilio/src/moz/gecko/layout/generic/nsPlaceholderFrame.cpp:181
#6 0x00005cc60e80cf19 in nsBlockFrame::DoRemoveFrameInternal (this=<optimized out>, aDeletedFrame=0x0, aFlags=<optimized out>, aPostDestroyData=...) at /home/emilio/src/moz/gecko/layout/generic/nsBlockFrame.cpp:6265
#7 0x00005cc60e82d947 in nsBlockFrame::DoRemoveFrame (this=0x4b1b39362d88, aDeletedFrame=0x683d8f00, aFlags=244338087) at /home/emilio/src/moz/gecko/layout/generic/nsBlockFrame.h:528
#8 0x00005cc60e80ba3a in nsBlockFrame::RemoveFrame (this=0x4b1b39362d88, aListID=<optimized out>, aOldFrame=0x4b1b39363240) at /home/emilio/src/moz/gecko/layout/generic/nsBlockFrame.cpp:5581
#9 0x00005cc60e77fd5c in nsCSSFrameConstructor::ContentRemoved (this=<optimized out>, aChild=0x4b1b3938b600, aOldNextSibling=<optimized out>, aFlags=<optimized out>)

	    at /home/emilio/src/moz/gecko/layout/base/nsCSSFrameConstructor.cpp:7583

#10 0x00005cc60e779a35 in nsCSSFrameConstructor::RecreateFramesForContent (this=0x6fdf9800, aContent=0x4b1b3938b600, aInsertionKind=nsCSSFrameConstructor::InsertionKind::Sync)

	    at /home/emilio/src/moz/gecko/layout/base/nsCSSFrameConstructor.cpp:8593

#11 0x00005cc60e751745 in mozilla::RestyleManager::ProcessRestyledFrames (this=<optimized out>, aChangeList=...) at /home/emilio/src/moz/gecko/layout/base/RestyleManager.cpp:1484

But the temporary state is stored in the _old_ frame-loader, so when we create
the new frame, we get to nsSubDocumentFrame::Init, and find nothing, and thus
go through nsFrameLoader::Show. But we do have a pres-shell, and
nsFrameLoader::Show just early-returns then, and thus we end up with a detached
pres shell which is not hooked to the view tree and thus not painted...

So there are multiple potential fixes. First (this is the approach this patch
takes):

Make nsHideViewer not fail to hide a presentation when the frame loader has changed. This is not an issue per se, but leaves stale views / etc living for too long which is not nice.
Fix up the Show() code path to handle this case properly by parenting the pres-shell and initializing the docshell properly.

Second potential fix would be to store the temporary state somewhere else than
the frame loader (like the element). This may be a less invasive change
somehow, but it looks pretty fishy to me, and not particularly better...

Terribly sorry about the lack of test-case, but this is racy as crazy and I had
a lot of trouble to even reproduce it in a debug build. This needs the
PresShell creation for the subdocument to happen right after setting .data on
the <object>, but before processing its reframe.

Diff Detail

Repository

rMOZILLACENTRAL mozilla-central

Build Status

Buildable 618419
Build 716712: arc lint + arc unit

Event Timeline

emilio created this revision.Apr 3 2020, 3:03 AM

Herald added a project: secure-revision. · View Herald TranscriptApr 3 2020, 3:03 AM

Harbormaster completed remote builds in B179712: Diff 254661.Apr 3 2020, 3:03 AM

phab-bot requested review of this revision.Apr 3 2020, 3:03 AM

phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".

phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".

phab-bot removed a project: secure-revision.

Code analysis found 1 defect in the diff 254661:

1 defect found by clang-tidy

You can run this analysis locally with:

./mach static-analysis check layout/generic/nsSubDocumentFrame.h (C/C++)

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

Hmm, so this is basically pulling the stashed views via a different path (from the docshell of the frame loader instead of from the detached views on the frame loader), and inserting the views back in at a different point in the code (nsFramerLoader::Show instead of nsSubdocumentFrame::Init). The one thing that is different though is that we are not checking that the container document is the same. I'm not sure why that is necessary but it was specifically asked for in the original bug (bug 775965, comment 6). So that makes me think it is important. Which makes me think we should move the stashing to the element so we can keep that check and continue having only one place that does the re-inserting of stashed views. What makes you think moving the stashed views to the element is fishy?

Even if you can't get a testcase to reliably reproduce I still think there is a lot of value in landing the testcase. We run tests in a lot of different configurations and so timing might change that could catch this bug regressing. I know I've seen bugs come and go in our testing infrastructure (ie bug 1582653 went away for months). Also, I know at one time our fuzzers used crashtests as seeds for their random fuzzing. The fact that this issue hasn't been found by fuzzing suggests this area hasn't really been tested by fuzzers, so if we add a testcase the fuzzing can maybe explore this area.

This isn't this bug but I'm pretty certain all of nsSubdocumentFrame hasn't been written with the fact that the frameloader can change in mind. Do you know if a frameloader change always causes a frame recreation of the nsSubdocumentFrame? Even a frame recreation might not be enough because you're suggesting that we might be doing some things with the nsSubdocumentFrame before it gets destroyed. So we might need a followup/audit for this.

In D69487#2111255, @tnikkel wrote:

Hmm, so this is basically pulling the stashed views via a different path (from the docshell of the frame loader instead of from the detached views on the frame loader), and inserting the views back in at a different point in the code (nsFramerLoader::Show instead of nsSubdocumentFrame::Init). The one thing that is different though is that we are not checking that the container document is the same. I'm not sure why that is necessary but it was specifically asked for in the original bug (bug 775965, comment 6). So that makes me think it is important.

It's not exactly the same. In particular, when we hit this case we still go down through the InitWindow code-path. Checking the document is important for the reframe case so that you don't end up accidentally exposing a document that shouldn't be exposed. But that shouldn't be an issue for this because frameloader changes cause a reframe.

Which makes me think we should move the stashing to the element so we can keep that check and continue having only one place that does the re-inserting of stashed views. What makes you think moving the stashed views to the element is fishy?

I think it's a bit fishy because we'd still somehow need to detect whether this docshell has been properly initialized or not to avoid early-returning.

Even if you can't get a testcase to reliably reproduce I still think there is a lot of value in landing the testcase. We run tests in a lot of different configurations and so timing might change that could catch this bug regressing. I know I've seen bugs come and go in our testing infrastructure (ie bug 1582653 went away for months). Also, I know at one time our fuzzers used crashtests as seeds for their random fuzzing. The fact that this issue hasn't been found by fuzzing suggests this area hasn't really been tested by fuzzers, so if we add a testcase the fuzzing can maybe explore this area.

Yeah, I'll try to write a test with the refresh-driver control mechanism... Let's see how far I get.

This isn't this bug but I'm pretty certain all of nsSubdocumentFrame hasn't been written with the fact that the frameloader can change in mind. Do you know if a frameloader change always causes a frame recreation of the nsSubdocumentFrame? Even a frame recreation might not be enough because you're suggesting that we might be doing some things with the nsSubdocumentFrame before it gets destroyed. So we might need a followup/audit for this.

Will file. But yeah, every change to the frameloader should cause a reframe, just not synchronously (which is the reason for this bug)... In this case it's done here via the loadingChanged condition.

In D69487#2117491, @emilio wrote:

In D69487#2111255, @tnikkel wrote:

Hmm, so this is basically pulling the stashed views via a different path (from the docshell of the frame loader instead of from the detached views on the frame loader), and inserting the views back in at a different point in the code (nsFramerLoader::Show instead of nsSubdocumentFrame::Init). The one thing that is different though is that we are not checking that the container document is the same. I'm not sure why that is necessary but it was specifically asked for in the original bug (bug 775965, comment 6). So that makes me think it is important.

It's not exactly the same. In particular, when we hit this case we still go down through the InitWindow code-path.

Ah, okay.

Checking the document is important for the reframe case so that you don't end up accidentally exposing a document that shouldn't be exposed. But that shouldn't be an issue for this because frameloader changes cause a reframe.

This confuses me. Important for the reframe case but okay because the frameloader change causes a reframe?

Could the containing document change simultaneously with the frame loader change? Probably exceedingly rare, but an attacker could try to force it to happen.

Which makes me think we should move the stashing to the element so we can keep that check and continue having only one place that does the re-inserting of stashed views. What makes you think moving the stashed views to the element is fishy?

I think it's a bit fishy because we'd still somehow need to detect whether this docshell has been properly initialized or not to avoid early-returning.

Okay.

Even if you can't get a testcase to reliably reproduce I still think there is a lot of value in landing the testcase. We run tests in a lot of different configurations and so timing might change that could catch this bug regressing. I know I've seen bugs come and go in our testing infrastructure (ie bug 1582653 went away for months). Also, I know at one time our fuzzers used crashtests as seeds for their random fuzzing. The fact that this issue hasn't been found by fuzzing suggests this area hasn't really been tested by fuzzers, so if we add a testcase the fuzzing can maybe explore this area.

Yeah, I'll try to write a test with the refresh-driver control mechanism... Let's see how far I get.

Just landing it as it should be fine.

Thinking about this some more, does your new view hooking up code in nsFrameLoader::Show actually get executed? Wouldn't the change to nsHideViewer mean that we call nsFrameLoader::Hide before the code in nsFrameLoader::Show could execute (and that would presumably destroy the presshell)?

Similar to the situation I mentioned above, would it be possible for the frameloader change the nsObjectLoadingContent does to happen simultaneously with a change to the remoteness of the frameloader?

In D69487#2131448, @tnikkel wrote:

Checking the document is important for the reframe case so that you don't end up accidentally exposing a document that shouldn't be exposed. But that shouldn't be an issue for this because frameloader changes cause a reframe.

This confuses me. Important for the reframe case but okay because the frameloader change causes a reframe?

Could the containing document change simultaneously with the frame loader change? Probably exceedingly rare, but an attacker could try to force it to happen.

I was confused here. The document check is not useful anywhere as unbinding from the document causes the frame loader to be destroyed. So I don't think that's a concern. I can probably remove the check in a follow-up.

Thinking about this some more, does your new view hooking up code in nsFrameLoader::Show actually get executed? Wouldn't the change to nsHideViewer mean that we call nsFrameLoader::Hide before the code in nsFrameLoader::Show could execute (and that would presumably destroy the presshell)?

It does get executed. Note that the frame loader in nsHideViewer is different from the one that we end up calling Show() on.

Similar to the situation I mentioned above, would it be possible for the frameloader change the nsObjectLoadingContent does to happen simultaneously with a change to the remoteness of the frameloader?

I'm not sure what you mean, mind clarifying?

Add testcase

Harbormaster completed remote builds in B183034: Diff 259329.Apr 12 2020, 9:15 PM

Code analysis found 1 defect in the diff 259329: