Closed Bug 1405176 Opened 2 years ago Closed 9 months ago

Assertion failure: aContentPolicyType == nsIContentPolicy::TYPE_DOCUMENT (DoURILoad thinks this is a document and InternalLoad does not) [@ nsDocShell::DoURILoad]

Categories

(Core :: DOM: Security, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox-esr60 --- wontfix
firefox58 --- wontfix
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- fixed

People

(Reporter: tsmith, Assigned: jkt)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase, Whiteboard: [domsecurity-active])

Attachments

(5 files)

Attached file test_case.html
STR:
1) Put both launcher.html and test_case.html in the same directory
2) open launcher.html

Assertion failure: aContentPolicyType == nsIContentPolicy::TYPE_DOCUMENT (DoURILoad thinks this is a document and InternalLoad does not), at /src/docshell/base/nsDocShell.cpp:11067

#0 nsDocShell::DoURILoad(nsIURI*, nsIURI*, mozilla::Maybe<nsCOMPtr<nsIURI> > const&, bool, bool, nsIURI*, bool, unsigned int, nsIPrincipal*, nsIPrincipal*, char const*, nsTSubstring<char16_t> const&, nsIInputStream*, long, nsIInputStream*, bool, nsIDocShell**, nsIRequest**, bool, bool, bool, nsTSubstring<char16_t> const&, nsIURI*, unsigned int) /src/docshell/base/nsDocShell.cpp:11241:16
#1 nsDocShell::InternalLoad(nsIURI*, nsIURI*, mozilla::Maybe<nsCOMPtr<nsIURI> > const&, bool, nsIURI*, unsigned int, nsIPrincipal*, nsIPrincipal*, unsigned int, nsTSubstring<char16_t> const&, char const*, nsTSubstring<char16_t> const&, nsIInputStream*, long, nsIInputStream*, unsigned int, nsISHEntry*, bool, nsTSubstring<char16_t> const&, nsIDocShell*, nsIURI*, bool, nsIDocShell**, nsIRequest**) /src/docshell/base/nsDocShell.cpp:10868:8
#2 nsDocShell::LoadURI(nsIURI*, nsIDocShellLoadInfo*, unsigned int, bool) /src/docshell/base/nsDocShell.cpp:1612:10
#3 nsFrameLoader::ReallyStartLoadingInternal() /src/dom/base/nsFrameLoader.cpp:985:19
#4 nsFrameLoader::ReallyStartLoading() /src/dom/base/nsFrameLoader.cpp:870:17
#5 nsDocument::MaybeInitializeFinalizeFrameLoaders() /src/dom/base/nsDocument.cpp:7556:13
#6 mozilla::detail::RunnableMethodImpl<nsDocument*, void (nsDocument::*)(), true, (mozilla::RunnableKind)0>::Run() /src/obj-firefox/dist/include/nsThreadUtils.h:1192:13
#7 nsContentUtils::AddScriptRunner(already_AddRefed<nsIRunnable>) /src/dom/base/nsContentUtils.cpp:5716:13
#8 nsContentUtils::AddScriptRunner(nsIRunnable*) /src/dom/base/nsContentUtils.cpp:5723:3
#9 nsDocument::InitializeFrameLoader(nsFrameLoader*) /src/dom/base/nsDocument.cpp:7498:5
#10 nsFrameLoader::LoadURI(nsIURI*) /src/dom/base/nsFrameLoader.cpp:352:13
#11 nsFrameLoader::LoadFrame() /src/dom/base/nsFrameLoader.cpp:298:10
#12 nsGenericHTMLFrameElement::LoadSrc() /src/dom/html/nsGenericHTMLFrameElement.cpp:250:31
#13 mozilla::dom::HTMLIFrameElement::OnAttrSetButNotChanged(int, nsIAtom*, nsAttrValueOrString const&, bool) /src/dom/html/HTMLIFrameElement.cpp:183:3
#14 mozilla::dom::Element::SetAttr(int, nsIAtom*, nsIAtom*, nsTSubstring<char16_t> const&, bool) /src/dom/base/Element.cpp:2509:12
#15 mozilla::dom::Element::SetAttr(nsIAtom*, nsTSubstring<char16_t> const&, mozilla::ErrorResult&) /src/obj-firefox/dist/include/mozilla/dom/Element.h:1391:14
#16 mozilla::dom::HTMLIFrameElementBinding::set_srcdoc(JSContext*, JS::Handle<JSObject*>, mozilla::dom::HTMLIFrameElement*, JSJitSetterCallArgs) /src/obj-firefox/dom/bindings/HTMLIFrameElementBinding.cpp:133:9
#17 mozilla::dom::GenericBindingSetter(JSContext*, unsigned int, JS::Value*) /src/dom/bindings/BindingUtils.cpp:3014:8
#18 js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) /src/js/src/jscntxtinlines.h:293:15
#19 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /src/js/src/vm/Interpreter.cpp:495:16
#20 InternalCall(JSContext*, js::AnyInvokeArgs const&) /src/js/src/vm/Interpreter.cpp:540:12
#21 js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /src/js/src/vm/Interpreter.cpp:559:10
#22 js::CallSetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<JS::Value>) /src/js/src/vm/Interpreter.cpp:688:12
#23 SetExistingProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<js::NativeObject*>, JS::Handle<JS::PropertyResult>, JS::ObjectOpResult&) /src/js/src/vm/NativeObject.cpp:2733:10
#24 bool js::NativeSetProperty<(js::QualifiedBool)1>(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&) /src/js/src/vm/NativeObject.cpp:2769:20
#25 js::SetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&) /src/js/src/vm/NativeObject.h:1615:12
#26 SetPropertyOperation(JSContext*, JSOp, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::Handle<JS::Value>) /src/js/src/vm/Interpreter.cpp:269:12
#27 Interpret(JSContext*, js::RunState&) /src/js/src/vm/Interpreter.cpp:2882:10
#28 js::RunScript(JSContext*, js::RunState&) /src/js/src/vm/Interpreter.cpp:435:12
#29 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /src/js/src/vm/Interpreter.cpp:513:15
#30 InternalCall(JSContext*, js::AnyInvokeArgs const&) /src/js/src/vm/Interpreter.cpp:540:12
#31 js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /src/js/src/vm/Interpreter.cpp:559:10
#32 JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /src/js/src/jsapi.cpp:2965:12
#33 mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) /src/obj-firefox/dom/bindings/EventHandlerBinding.cpp:260:37
#34 void mozilla::dom::EventHandlerNonNull::Call<nsISupports*>(nsISupports* const&, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JSCompartment*) /src/obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:362:12
#35 mozilla::JSEventHandler::HandleEvent(nsIDOMEvent*) /src/dom/events/JSEventHandler.cpp:215:12
#36 mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) /src/dom/events/EventListenerManager.cpp:1112:51
#37 mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) /src/dom/events/EventListenerManager.cpp:1283:20
#38 mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) /src/dom/events/EventDispatcher.cpp:313:17
#39 mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /src/dom/events/EventDispatcher.cpp:462:16
#40 mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) /src/dom/events/EventDispatcher.cpp:822:9
#41 (anonymous namespace)::AsyncTimeEventRunner::Run() /src/dom/smil/nsSMILTimedElement.cpp:115:14
#42 nsThread::ProcessNextEvent(bool, bool*) /src/xpcom/threads/nsThread.cpp:1039:14
#43 NS_ProcessNextEvent(nsIThread*, bool) /src/xpcom/threads/nsThreadUtils.cpp:524:10
#44 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:97:21
#45 MessageLoop::RunInternal() /src/ipc/chromium/src/base/message_loop.cc:326:10
#46 MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:299:3
#47 nsBaseAppShell::Run() /src/widget/nsBaseAppShell.cpp:158:27
#48 nsAppStartup::Run() /src/toolkit/components/startup/nsAppStartup.cpp:288:30
#49 XREMain::XRE_mainRun() /src/toolkit/xre/nsAppRunner.cpp:4701:22
#50 XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4865:8
#51 XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4960:21
#52 do_main(int, char**, char**) /src/browser/app/nsBrowserApp.cpp:231:22
#53 main /src/browser/app/nsBrowserApp.cpp:304:16
#54 __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
#55 _start (firefox+0x41eb24)
Attached file launcher.html
Looks like Tanvi wrote the asserting code over in bug 1105556.
Component: Document Navigation → DOM: Security
Tanvi, can you take look at this one?
It seems there is an inconsistency, when we set the contentpolicytype we check for
  if (IsFrame() && !isTargetTopLevelDocShell) { see [1]
but when we do the assertion we only check for:
  if (IsFrame()) { see [2]

Probably the bug is somewhere around those bits, but someone needs to investigate closer.

[1] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#9950
[2] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#11045
Ethan, can someone from you team look at this?
Flags: needinfo?(ettseng)
Assignee: nobody → tnguyen
Flags: needinfo?(ettseng)
I could see the flow

  if (IsFrame() && !isTargetTopLevelDocShell) {
  } else {
    contentType = TYPE_DOCUMENT;
  }

  ....DO STUFF...

  if (IsFrame()) {
  } else {
    *Assert not document contentType != TYPE_DOCUMENT*
  }

Obviously, there's something wrong in IsFrame() (it is expected to be true, but actual value is false), 
https://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/docshell/base/nsDocShell.cpp#13726
https://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/docshell/base/nsDocShell.cpp#3516
I am following the step in comment 0, unfortunately I could not reproduce that.
Hi Tyson, could you please tell me more details about how you see the issue, like your version, platform, etc? As my running, I leave my machine running for 15 mins (the test opens a new tab then close it), but I don't see the issue.
Thanks
Flags: needinfo?(twsmith)
Yeah, the assertion should never hit.  So it would be good to figure out in what scenario it is hitting.  Once we know that, we can take a look, and perhaps loop in BZ, to see if it is a valid scenario or there is a bug somewhere else.  Or if the assertion needs to be adjusted.
Attached file prefs.js
Using this prefs.js helps make the test case more reliable. I verified this is reproducible with the latest m-c build (tested on Ubuntu 16.04).
Flags: needinfo?(twsmith)
(In reply to Tyson Smith [:tsmith] from comment #8)
> Created attachment 8921278 [details]
> prefs.js
> 
> Using this prefs.js helps make the test case more reliable. I verified this
> is reproducible with the latest m-c build (tested on Ubuntu 16.04).

Thanks Tyson
Likely docShell is being destroyed but about:srcdoc is still being loaded in child docshell.
I could see mParent is null and IsFrame() is wrong as comment 6
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
The interesting point is the parent is going away in the middle of the load (between entry to InternalLoad and calling into the DoURILoad to actually start the load).
I spent some time looking into that and I found that the Stop call at [1] runs event handlers which would remove a frame in the middle of a load.  We start destroying its FrameLoader (StartDestroy) and defer destroying the docshell(DestroyDocShell in nsFrameLoaderDestroyRunnable) until we return to the event loop. We continue starting load DoURILoad but the docshell will be destroyed right after that.
I think we should bail out in that case.

[1] https://searchfox.org/mozilla-central/rev/1285ae3e810e2dbf2652c49ee539e3199fcfe820/docshell/base/nsDocShell.cpp#10784

The patch works for me for the STR in comment 8.
Comment on attachment 8921832 [details]
Bug 1405176 - Bail out DoURILoad if the docshell is being async destructed

https://reviewboard.mozilla.org/r/192858/#review198080

::: docshell/base/nsDocShell.cpp:10792
(Diff revision 1)
>  
>      if (NS_FAILED(rv)) {
>        return rv;
>      }
> +
> +    if (contentType == nsIContentPolicy::TYPE_INTERNAL_FRAME ||

Why couldn't this same thing happen with top level docshells too. unload listener closes the top level window or so.
Attachment #8921832 - Flags: review?(bugs) → review-
Comment on attachment 8921832 [details]
Bug 1405176 - Bail out DoURILoad if the docshell is being async destructed

https://reviewboard.mozilla.org/r/192858/#review198080

> Why couldn't this same thing happen with top level docshells too. unload listener closes the top level window or so.

That is a good point, I don't have any idea if there's any real scenario we would encounter that (destroy top level in the middle of the load).
Perhaps, we could add a check like  if (contentType == nsIContentPolicy::TYPE_DOCUMENT && !mScriptGlobal->AsOuter()) {
  return;
} but not fully sure whether it will cover all the cases we may encounter or not.
Looking again into how does the issue occur, https://searchfox.org/mozilla-central/rev/dd47bee6468de7e1221b4d006342ad6b9813d0e5/dom/base/nsFrameLoader.cpp#2240

mNeedsAsyncDestroy is set to true if we are in the middle of the load. I think I can do a bit more to create a "asyncDestroyed" variable in docshell to bail out loading early
Thanks for your review :).
Could you please take a look at new approach?
Now I'm a bit lost. Why the new patch is totally different comparing to the first one?
Could you explain a bit.
(In reply to Olli Pettay [:smaug] from comment #16)
> Now I'm a bit lost. Why the new patch is totally different comparing to the
> first one?
> Could you explain a bit.

Right, that is a different approach. 
The first approach, you could see what we are doing in StartDestroy
https://searchfox.org/mozilla-central/rev/dd47bee6468de7e1221b4d006342ad6b9813d0e5/dom/base/nsFrameLoader.cpp#2220
that will remove the frame element. Then after stoping and calling event hanlders, we could check if the frame element is removed (that mean we are in destruction) to bail out. 
This approach would not cover the toplevel case (although I am not quite sure whether there's any real scenario or not) as you mentioned. We could update this approach a by adding a condition check like comment 13. But seems we do not guarantee this approach will cover all corner cases

The issue in this bug happens when we have a defer docshell destroying in the middle of a load, that makes me think about the second approach. That is we could bail out DoURILoad if we are in "async docshell destruction" (even we can bail out a little bit earlier). I think the second is better one
(In reply to Thomas Nguyen[:tnguyen] ni? plz from comment #17)

> The issue in this bug happens when we have a defer docshell destroying in
> the middle of a load, that makes me think about the second approach. That is
> we could bail out DoURILoad if we are in "async docshell destruction" (even
> we can bail out a little bit earlier).

I mean we could use a variable to save aysnc destruction state because we know exactly when it could happen
https://searchfox.org/mozilla-central/rev/dd47bee6468de7e1221b4d006342ad6b9813d0e5/dom/base/nsFrameLoader.cpp#2240
Comment on attachment 8921832 [details]
Bug 1405176 - Bail out DoURILoad if the docshell is being async destructed

https://reviewboard.mozilla.org/r/192858/#review198638

I don't like this approach, since this can't deal at all the top level browsing context case.
Attachment #8921832 - Flags: review?(bugs) → review-
For top level case it is probably enough to check if  mIsBeingDestroyed is true.
(In reply to Olli Pettay [:smaug] from comment #20)
> For top level case it is probably enough to check if  mIsBeingDestroyed is
> true.

Thanks Smaug for looking at the patch.
Let me clarify/summary this bug:
- 1 - This bug occurred if we found a zombie doc when we were trying to load uri in a frame (frame A)
https://searchfox.org/mozilla-central/rev/1ebd2eff44617df3b82eea7d2f3ca1b60cc591a0/docshell/base/nsDocShell.cpp#10781
and then try to call Stop() which will trigger event handlers and destroy frameLoader.
- 2 - Firstly, nsFrameLoader of parent frame will be destroyed and then the parent docshell is destroyed *synchronously* 
(in doc->FinalizeFrameLoader)
https://searchfox.org/mozilla-central/rev/1ebd2eff44617df3b82eea7d2f3ca1b60cc591a0/dom/base/nsFrameLoader.cpp#2239
https://searchfox.org/mozilla-central/rev/1ebd2eff44617df3b82eea7d2f3ca1b60cc591a0/dom/base/nsFrameLoader.cpp#2313
- 3 - Destroying parent docshell will null out the back ptr of children bound to parent
https://searchfox.org/mozilla-central/rev/1ebd2eff44617df3b82eea7d2f3ca1b60cc591a0/uriloader/base/nsDocLoader.cpp#371
- 4 - Secondly, nsFrameLoader of A frame (children docshell) will be destroyed, but deferred in a runnable
(NS_DispatchToCurrentThread(destroyRunnable))

https://searchfox.org/mozilla-central/rev/1ebd2eff44617df3b82eea7d2f3ca1b60cc591a0/dom/base/nsFrameLoader.cpp#2240
- 5 - Continue loading, from that time the check IsFrame will be wrong because the parent is null in step 3 then we stop at the assertion.
 
We could fix the problem like comment 12, but we are going to extend the fix to the top level context to avoid similar thing may happen. Something we've had:
- We have mIsBeingDestroyed to bail out somewhere so it would be ok the docshell is being destroyed.
- If there's any unexpected scenario like 1 - 4 - 5 like top level docshell is async destroyed and we are continuing to load channel. IsFrame (==false) is still right and we can continue loading as usually. If you think it's still worth to bail out here, we could not use mIsBeingDestroyed (since it's still false). We may not want to set mIsBeingDestroyed to true to early. It would be risky :( because I guess it may break unload event order after destroying docshell.
Smaug, I need to consult you again, what do you think about that? Please correct me if I am wrong or missing anything?
Flags: needinfo?(bugs)
(In reply to Thomas Nguyen[:tnguyen] ni? plz from comment #21)

> - 2 - Firstly, nsFrameLoader of parent frame will be destroyed and then the
> parent docshell is destroyed *synchronously* 
> (in doc->FinalizeFrameLoader)
> https://searchfox.org/mozilla-central/rev/
> 1ebd2eff44617df3b82eea7d2f3ca1b60cc591a0/dom/base/nsFrameLoader.cpp#2239
> https://searchfox.org/mozilla-central/rev/
> 1ebd2eff44617df3b82eea7d2f3ca1b60cc591a0/dom/base/nsFrameLoader.cpp#2313

If you mean top level page here, there is no nsFrameLoader for the top level page in multiprocess setup.


> - We have mIsBeingDestroyed to bail out somewhere so it would be ok the
> docshell is being destroyed.
> - If there's any unexpected scenario like 1 - 4 - 5 like top level docshell
> is async destroyed and we are continuing to load channel. IsFrame (==false)
> is still right and we can continue loading as usually. If you think it's
> still worth to bail out here, we could not use mIsBeingDestroyed (since it's
> still false). We may not want to set mIsBeingDestroyed to true to early.
We don't. mIsBeingDestroyed would be used for top level pages only. It would be still false
for iframes in this case.
Flags: needinfo?(bugs)
Assignee: tnguyen → nobody
Status: ASSIGNED → NEW
There are 24 total failures in the last 7 days, mostly on windows10-64-ccov. 

Recent failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=218734909&repo=mozilla-central&lineNumber=25243

01:53:54     INFO - TEST-START | /html/webappapis/dynamic-markup-insertion/opening-the-input-stream/ignore-opens-during-unload.window.html

01:53:57     INFO - PID 608 | [Child 4044, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, false) failed with result 0x80004005: file z:/build/build/src/docshell/shistory/nsSHistory.cpp, line 1198
01:53:57     INFO - PID 608 | [Child 4044, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, false) failed with result 0x80004005: file z:/build/build/src/docshell/shistory/nsSHistory.cpp, line 1198
01:53:57     INFO - PID 608 | [Child 4044, Main Thread] WARNING: NS_ENSURE_TRUE(mState == WCC_ONWRITE) failed: file z:/build/build/src/netwerk/protocol/wyciwyg/WyciwygChannelChild.cpp, line 665
01:53:58     INFO - PID 608 | --DOCSHELL 0000016806B7B800 == 3 [pid = 5680] [id = {64ad6321-26fa-4134-b890-053a9236b906}]
01:53:58     INFO - PID 608 | Assertion failure: aContentPolicyType == nsIContentPolicy::TYPE_DOCUMENT (DoURILoad thinks this is a document and InternalLoad does not), at z:/build/build/src/docshell/base/nsDocShell.cpp:9779
01:53:59     INFO - mozcrash Copy/paste: Z:\task_1545698575\build\win32-minidump_stackwalk.exe c:\users\task_1545698575\appdata\local\temp\tmpwpqsbh.mozrunner\minidumps\02df847a-d930-4650-b493-8c664a0dc0ca.dmp Z:\task_1545698575\build\symbols
01:54:08     INFO - mozcrash Saved minidump as Z:\task_1545698575\build\blobber_upload_dir\02df847a-d930-4650-b493-8c664a0dc0ca.dmp
01:54:08     INFO - mozcrash Saved app info as Z:\task_1545698575\build\blobber_upload_dir\02df847a-d930-4650-b493-8c664a0dc0ca.extra

Christoph can you assign someone to take a look?
Flags: needinfo?(ckerschb)
(In reply to Andreea Pavel [:apavel] from comment #45)
> Christoph can you assign someone to take a look?

Jonathan, this is closely related to providing the correct principal for top-level loads. Please take a look!
Flags: needinfo?(ckerschb) → needinfo?(jkt)
Taking this as looking into it closely. I can reproduce the debug failure of /html/webappapis/dynamic-markup-insertion/opening-the-input-stream/ignore-opens-during-unload.window.html. The previous patch also solves this however I'm not sure if it's the correct solution without further investigation.
Assignee: nobody → jkt
Flags: needinfo?(jkt)

Hey Nika, we discussed this bug last week and I have been playing around with passing SetIsFrame through docshell from the FrameLoader code:

   // Now that we are part of the DocShellTree, attach our DocShell to our
   // parent's TreeOwner.
   nsCOMPtr<nsIDocShellTreeOwner> parentTreeOwner;
   parentDocShell->GetTreeOwner(getter_AddRefs(parentTreeOwner));
   AddTreeItemToTreeOwner(mDocShell, parentTreeOwner);
 
+    nsCOMPtr<nsIDocShellTreeItem> parentCheck;
+    mDocShell->GetSameTypeParent(getter_AddRefs(parentCheck));
+    if (!!parentCheck) {
+      mDocShell->SetIsFrame();
+    }
+

Essentially this is checking for when is not a mozbrowser and both parent and child are in the same type.

https://hg.mozilla.org/try/rev/59046070d5eb6c0494b266d46c6a3c9418dc6447 is an example try push (excuse the tonne of debug please). However I'm getting some odd failures of some small number of content mozbrowser iframes not loading as they end up trying to use a null principal.

Is there anything that is obviously wrong you can think of that might cause this issue?

Flags: needinfo?(nika)
Pushed by jkingston@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0c9623b98c22
Change IsFrame() in nsDocShell to be an explicitly passed attribute. r=nika
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

I answered this on the review :-)

Flags: needinfo?(nika)
You need to log in before you can comment on or make changes to this bug.