1108547 - Private browsing mode context is broken by <a> or <form> with target attribute

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[Muneaki Nishimura]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/40.0.2214.28 Safari/537.36

Steps to reproduce:

1) Launch following page from normal browsing mode in order to set cookie
http://csrf.jp/cookie/set.php?domain=csrf.jp&name=NON-PRIVATE&value=MODE-COOKIE
2) Open private browsing mode and launch following page
http://csrf.jp/target.html
3) Click "Non-Private Mode Cookie is Used" link or button


Actual results:

At procedure 3), non-browsing mode cookie is shown on an alert dialog.


Expected results:

At procedure 3), private mode cookie has to be shown.
Note that the text/button of "Non-Private Mode Cookie is Used" has target attribute.

Comment 1

[Daniel Veditz [:dveditz]]

I confirm this behavior in nightly: even though the new page is a tab in a window labelled with the private browsing icon, it is NOT a private tab. Of course putting it in a correctly labelled window isn't the solution because this lets private pages collaborate with their public counter-parts.

Ehsan: can you help route this bug to whomever owns private browsing these days?

Comment 2

[Muneaki Nishimura]

Here is the supplement on the bug:
I'm not sure whether the cause is same but hyperlink with 'target' attribute can access other window/frame beyond the private browsing mode like below.

1) Open http://alice.csrf.jp/iframe/public.html on normal mode.
2) Open http://alice.csrf.jp/iframe/private.html on private mode.
3) Click a link 'Change Public Frame' on 2).
4) Child page in 1) is changed beyond the mode.

The opposite is equally possible, i.e., private window can be controlled from normal mode.

Comment 3

[:Gijs (he/him)]

(In reply to Muneaki Nishimura from comment #2)
> Here is the supplement on the bug:
> I'm not sure whether the cause is same but hyperlink with 'target' attribute
> can access other window/frame beyond the private browsing mode like below.
> 
> 1) Open http://alice.csrf.jp/iframe/public.html on normal mode.
> 2) Open http://alice.csrf.jp/iframe/private.html on private mode.
> 3) Click a link 'Change Public Frame' on 2).
> 4) Child page in 1) is changed beyond the mode.
> 
> The opposite is equally possible, i.e., private window can be controlled
> from normal mode.

This is covered by bug 1100154, I think.

Josh, I know you made that a mentored bug, but this seems like it's more serious and might be caused by the same issue... in which case, do you have time to look at this?

Comment 5

[(no longer active)]

I don't think this is a security bug.  Is it OK to open it up?

Comment 6

[(no longer active)]

(In reply to :Gijs Kruitbosch from comment #3)
> (In reply to Muneaki Nishimura from comment #2)
> > Here is the supplement on the bug:
> > I'm not sure whether the cause is same but hyperlink with 'target' attribute
> > can access other window/frame beyond the private browsing mode like below.
> > 
> > 1) Open http://alice.csrf.jp/iframe/public.html on normal mode.
> > 2) Open http://alice.csrf.jp/iframe/private.html on private mode.
> > 3) Click a link 'Change Public Frame' on 2).
> > 4) Child page in 1) is changed beyond the mode.
> > 
> > The opposite is equally possible, i.e., private window can be controlled
> > from normal mode.
> 
> This is covered by bug 1100154, I think.

I'm pretty sure these are both the same underlying issue, but will know for sure when I finish my fix for this bug.

Comment 7

[(no longer active)]

The problem here seems to be how the docshell for the new tab is initialized:

* thread #1: tid = 0x3d20d0, 0x000000010559fd67 XUL`nsDocShell::SetDocLoaderParent(this=0x000000011e1af800, aParent=0x00000001003270b0) + 1719 at nsDocShell.cpp:3399, queue = 'com.apple.main-thread', stop reason = step over
    frame #0: 0x000000010559fd67 XUL`nsDocShell::SetDocLoaderParent(this=0x000000011e1af800, aParent=0x00000001003270b0) + 1719 at nsDocShell.cpp:3399
    frame #1: 0x0000000102817a04 XUL`nsDocLoader::AddChildLoader(this=0x00000001003270b0, aChild=0x000000011e1af800) + 148 at nsDocLoader.cpp:644
  * frame #2: 0x0000000102817947 XUL`nsDocLoader::AddDocLoaderAsChildOfRoot(aDocLoader=0x000000011e1af800) + 391 at nsDocLoader.cpp:248
    frame #3: 0x000000010558c208 XUL`nsDocShell::Init(this=0x000000011e1af800) + 888 at nsDocShell.cpp:954
    frame #4: 0x00000001055f219d XUL`nsDocShellConstructor(aOuter=0x0000000000000000, aIID=0x0000000107749138, aResult=0x00007fff5fbf2440) + 205 at nsDocShellModule.cpp:68
    frame #5: 0x0000000101736cde XUL`mozilla::GenericFactory::CreateInstance(this=0x000000011a803b00, aOuter=0x0000000000000000, aIID=0x0000000107749138, aResult=0x00007fff5fbf2440) + 46 at GenericFactory.cpp:17
    frame #6: 0x00000001016d2362 XUL`nsComponentManagerImpl::CreateInstanceByContractID(this=0x00000001003264e0, aContractID=0x00000001084bbe67, aDelegate=0x0000000000000000, aIID=0x0000000107749138, aResult=0x00007fff5fbf2440) + 338 at nsComponentManager.cpp:1199
    frame #7: 0x000000010173b42b XUL`CallCreateInstance(aContractID=0x00000001084bbe67, aDelegate=0x0000000000000000, aIID=0x0000000107749138, aResult=0x00007fff5fbf2440) + 139 at nsComponentManagerUtils.cpp:149
    frame #8: 0x000000010173b634 XUL`nsCreateInstanceByContractID::operator(this=0x00007fff5fbf2938, aIID=0x0000000107749138, aInstancePtr=0x00007fff5fbf2440)(nsID const&, void**) const + 52 at nsComponentManagerUtils.cpp:197
    frame #9: 0x0000000102822e01 XUL`nsCOMPtr<nsIDocShell>::assign_from_helper(this=0x000000011a26b8a8, helper=0x00007fff5fbf2938, aIID=0x0000000107749138) + 65 at nsCOMPtr.h:1109
    frame #10: 0x0000000103019cc6 XUL`nsCOMPtr<nsIDocShell>::operator=(this=0x000000011a26b8a8, aRhs=0x00007fff5fbf2938) + 54 at nsCOMPtr.h:622
    frame #11: 0x0000000102fd652a XUL`nsFrameLoader::MaybeCreateDocShell(this=0x000000011a26b880) + 650 at nsFrameLoader.cpp:1612
    frame #12: 0x0000000102fd54dd XUL`nsFrameLoader::CheckURILoad(this=0x000000011a26b880, aURI=0x000000011bd1e9d0) + 173 at nsFrameLoader.cpp:490
    frame #13: 0x0000000102fd5219 XUL`nsFrameLoader::LoadURI(this=0x000000011a26b880, aURI=0x000000011bd1e9d0) + 233 at nsFrameLoader.cpp:287
    frame #14: 0x0000000102fd4fff XUL`nsFrameLoader::LoadFrame(this=0x000000011a26b880) + 1231 at nsFrameLoader.cpp:253
    frame #15: 0x000000010499f17d XUL`nsXULElement::LoadSrc(this=0x000000011bd1dbc0) + 477 at nsXULElement.cpp:1609
    frame #16: 0x000000010499ee03 XUL`nsXULElement::BindToTree(this=0x000000011bd1dbc0, aDocument=0x000000011c38e000, aParent=0x000000011bd1e790, aBindingParent=0x000000011e2fc6b0, aCompileEventHandlers=true) + 675 at nsXULElement.cpp:882
    frame #17: 0x0000000102e2da70 XUL`mozilla::dom::Element::BindToTree(this=0x000000011bd1e790, aDocument=0x000000011c38e000, aParent=0x000000011bd1e820, aBindingParent=0x000000011e2fc6b0, aCompileEventHandlers=true) + 3104 at Element.cpp:1539
    frame #18: 0x000000010499ec3b XUL`nsXULElement::BindToTree(this=0x000000011bd1e790, aDocument=0x000000011c38e000, aParent=0x000000011bd1e820, aBindingParent=0x000000011e2fc6b0, aCompileEventHandlers=true) + 219 at nsXULElement.cpp:840
    frame #19: 0x0000000102e2da70 XUL`mozilla::dom::Element::BindToTree(this=0x000000011bd1e820, aDocument=0x000000011c38e000, aParent=0x000000011bd1e8b0, aBindingParent=0x000000011e2fc6b0, aCompileEventHandlers=true) + 3104 at Element.cpp:1539
    frame #20: 0x000000010499ec3b XUL`nsXULElement::BindToTree(this=0x000000011bd1e820, aDocument=0x000000011c38e000, aParent=0x000000011bd1e8b0, aBindingParent=0x000000011e2fc6b0, aCompileEventHandlers=true) + 219 at nsXULElement.cpp:840
    frame #21: 0x0000000102e2da70 XUL`mozilla::dom::Element::BindToTree(this=0x000000011bd1e8b0, aDocument=0x000000011c38e000, aParent=0x000000011bd1e940, aBindingParent=0x000000011e2fc6b0, aCompileEventHandlers=true) + 3104 at Element.cpp:1539
    frame #22: 0x000000010499ec3b XUL`nsXULElement::BindToTree(this=0x000000011bd1e8b0, aDocument=0x000000011c38e000, aParent=0x000000011bd1e940, aBindingParent=0x000000011e2fc6b0, aCompileEventHandlers=true) + 219 at nsXULElement.cpp:840
    frame #23: 0x0000000102e2da70 XUL`mozilla::dom::Element::BindToTree(this=0x000000011bd1e940, aDocument=0x000000011c38e000, aParent=0x000000011c1f8040, aBindingParent=0x000000011e2fc6b0, aCompileEventHandlers=true) + 3104 at Element.cpp:1539
    frame #24: 0x000000010499ec3b XUL`nsXULElement::BindToTree(this=0x000000011bd1e940, aDocument=0x000000011c38e000, aParent=0x000000011c1f8040, aBindingParent=0x000000011e2fc6b0, aCompileEventHandlers=true) + 219 at nsXULElement.cpp:840
    frame #25: 0x0000000103000870 XUL`nsINode::doInsertChildAt(this=0x000000011c1f8040, aKid=0x000000011bd1e940, aIndex=1, aNotify=true, aChildArray=0x000000011c1f80b0) + 1232 at nsINode.cpp:1525
    frame #26: 0x0000000102e4c209 XUL`mozilla::dom::FragmentOrElement::InsertChildAt(this=0x000000011c1f8040, aKid=0x000000011bd1e940, aIndex=1, aNotify=true) + 137 at FragmentOrElement.cpp:1140
    frame #27: 0x0000000103002b87 XUL`nsINode::ReplaceOrInsertBefore(this=0x000000011c1f8040, aReplace=false, aNewChild=0x000000011bd1e940, aRefChild=0x0000000000000000, aError=0x00007fff5fbf3dc8) + 5383 at nsINode.cpp:2209
    frame #28: 0x0000000102e620b4 XUL`nsINode::InsertBefore(this=0x000000011c1f8040, aNode=0x000000011bd1e940, aChild=0x0000000000000000, aError=0x00007fff5fbf3dc8) + 52 at nsINode.h:1600
    frame #29: 0x0000000102e620f2 XUL`nsINode::AppendChild(this=0x000000011c1f8040, aNode=0x000000011bd1e940, aError=0x00007fff5fbf3dc8) + 50 at nsINode.h:1604
    frame #30: 0x000000010386ffdb XUL`mozilla::dom::NodeBinding::appendChild(cx=0x0000000125dc9350, obj=Handle<JSObject *> at 0x00007fff5fbf3e20, self=0x000000011c1f8040, args=0x00007fff5fbf3e90) + 379 at NodeBinding.cpp:598
    frame #31: 0x0000000103d70134 XUL`mozilla::dom::GenericBindingMethod(cx=0x0000000125dc9350, argc=1, vp=0x0000000116ef5460) + 660 at BindingUtils.cpp:2434
    frame #32: 0x000000010727e794 XUL`js::CallJSNative(cx=0x0000000125dc9350, native=0x0000000103d6fea0, args=0x00007fff5fbf4520)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 164 at jscntxtinlines.h:231
    frame #33: 0x000000010720028d XUL`js::Invoke(cx=0x0000000125dc9350, args=CallArgs at 0x00007fff5fbf4520, construct=NO_CONSTRUCT) + 1261 at Interpreter.cpp:484
    frame #34: 0x0000000107221013 XUL`Interpret(cx=0x0000000125dc9350, state=0x00007fff5fbf7140) + 51379 at Interpreter.cpp:2541
    frame #35: 0x000000010721470a XUL`js::RunScript(cx=0x0000000125dc9350, state=0x00007fff5fbf7140) + 666 at Interpreter.cpp:434
    frame #36: 0x00000001072003d1 XUL`js::Invoke(cx=0x0000000125dc9350, args=CallArgs at 0x00007fff5fbf7630, construct=NO_CONSTRUCT) + 1585 at Interpreter.cpp:503
    frame #37: 0x00000001071eae24 XUL`js::Invoke(cx=0x0000000125dc9350, thisv=0x00007fff5fbf7840, fval=0x00007fff5fbf80c0, argc=4, argv=0x00007fff5fbf82e8, rval=JS::MutableHandleValue at 0x00007fff5fbf7730) + 900 at Interpreter.cpp:540
    frame #38: 0x0000000106fb6921 XUL`JS_CallFunctionValue(cx=0x0000000125dc9350, obj=JS::HandleObject at 0x00007fff5fbf7888, fval=JS::HandleValue at 0x00007fff5fbf7880, args=0x00007fff5fbf7dc0, rval=JS::MutableHandleValue at 0x00007fff5fbf7878) + 337 at jsapi.cpp:4931
    frame #39: 0x0000000102479b98 XUL`nsXPCWrappedJSClass::CallMethod(this=0x0000000126129a10, wrapper=0x0000000126230c80, methodIndex=3, info_=0x000000011690d688, nativeParams=0x00007fff5fbf8550) + 6392 at XPCWrappedJSClass.cpp:1187
    frame #40: 0x0000000102473a1b XUL`nsXPCWrappedJS::CallMethod(this=0x0000000126230c80, methodIndex=3, info=0x000000011690d688, params=0x00007fff5fbf8550) + 203 at XPCWrappedJS.cpp:532
    frame #41: 0x0000000101716f16 XUL`PrepareAndDispatch(self=0x000000011e3af800, methodIndex=3, args=0x00007fff5fbf8670, gpregs=0x00007fff5fbf85f0, fpregs=0x00007fff5fbf8620) + 1654 at xptcstubs_x86_64_darwin.cpp:122
    frame #42: 0x000000010171592b XUL`SharedStub + 91
    frame #43: 0x000000010566d6a1 XUL`nsContentTreeOwner::ProvideWindow(this=0x000000011c1f8430, aParent=0x000000011e579420, aChromeFlags=66766, aCalledFromJS=false, aPositionSpecified=false, aSizeSpecified=false, aURI=0x000000011c1ad080, aName=0x00007fff5fbf91b0, aFeatures=0x00007fff5fbf9330, aWindowIsNew=0x00007fff5fbf9260, aReturn=0x00007fff5fbf9068) + 2001 at nsContentTreeOwner.cpp:922
    frame #44: 0x000000010566d820 XUL`non-virtual thunk to nsContentTreeOwner::ProvideWindow(this=0x000000011c1f8450, aParent=0x000000011e579420, aChromeFlags=66766, aCalledFromJS=false, aPositionSpecified=false, aSizeSpecified=false, aURI=0x000000011c1ad080, aName=0x00007fff5fbf91b0, aFeatures=0x00007fff5fbf9330, aWindowIsNew=0x00007fff5fbf9260, aReturn=0x00007fff5fbf9068) + 208 at nsContentTreeOwner.cpp:925
    frame #45: 0x00000001055f6dc8 XUL`nsWindowWatcher::OpenWindowInternal(this=0x00000001183a0e00, aParent=0x000000011e579420, aUrl=0x00000001260abbe8, aName=0x00007fff5fbf98a0, aFeatures=0x00007fff5fbf9900, aCalledFromJS=false, aDialog=false, aNavigate=false, aOpeningTab=0x0000000000000000, argv=0x0000000000000000, _retval=0x00007fff5fbf9770) + 3992 at nsWindowWatcher.cpp:627
    frame #46: 0x00000001055f907c XUL`nsWindowWatcher::OpenWindow2(this=0x00000001183a0e00, aParent=0x000000011e579420, aUrl=0x00000001260abbe8, aName=0x00007fff5fbf98a0, aFeatures=0x00007fff5fbf9900, aCalledFromScript=false, aDialog=false, aNavigate=false, aOpeningTab=0x0000000000000000, aArguments=0x0000000000000000, _retval=0x00007fff5fbf9770) + 476 at nsWindowWatcher.cpp:418
    frame #47: 0x00000001055f917e XUL`non-virtual thunk to nsWindowWatcher::OpenWindow2(this=0x00000001183a0e08, aParent=0x000000011e579420, aUrl=0x00000001260abbe8, aName=0x00007fff5fbf98a0, aFeatures=0x00007fff5fbf9900, aCalledFromScript=false, aDialog=false, aNavigate=false, aOpeningTab=0x0000000000000000, aArguments=0x0000000000000000, _retval=0x00007fff5fbf9770) + 206 at nsWindowWatcher.cpp:421
    frame #48: 0x0000000102d7e765 XUL`nsGlobalWindow::OpenInternal(this=0x000000011e579400, aUrl=0x00007fff5fbfa2f0, aName=0x00007fff5fbfa438, aOptions=0x00007fff5fbfa390, aDialog=false, aContentModal=false, aCalledNoScript=true, aDoJSFixups=false, aNavigate=false, argv=0x0000000000000000, aExtraArgument=0x0000000000000000, aCalleePrincipal=0x000000011c0faf10, aJSCallerContext=0x0000000000000000, aReturn=0x00007fff5fbfa430) + 3013 at nsGlobalWindow.cpp:11839
    frame #49: 0x0000000102d7f174 XUL`nsGlobalWindow::OpenNoNavigate(this=0x000000011e579400, aUrl=0x00007fff5fbfa2f0, aName=0x00007fff5fbfa438, aOptions=0x00007fff5fbfa390, _retval=0x00007fff5fbfa430) + 308 at nsGlobalWindow.cpp:7677
    frame #50: 0x0000000102d7f1c7 XUL`non-virtual thunk to nsGlobalWindow::OpenNoNavigate(this=0x000000011e579420, aUrl=0x00007fff5fbfa2f0, aName=0x00007fff5fbfa438, aOptions=0x00007fff5fbfa390, _retval=0x00007fff5fbfa430) + 71 at nsGlobalWindow.cpp:7688
    frame #51: 0x00000001055940c0 XUL`nsDocShell::InternalLoad(this=0x000000011df84000, aURI=0x0000000125d78900, aReferrer=0x00000001333c9400, aReferrerPolicy=0, aOwner=0x000000011c0faf10, aFlags=0, aWindowTarget=0x00007fff5fbfaae8, aTypeHint=0x000000010761005c, aFileName=0x000000010a1b1818, aPostData=0x000000011e374900, aHeadersData=0x0000000000000000, aLoadType=2097153, aSHEntry=0x0000000000000000, aFirstParty=true, aSrcdoc=0x000000010a1b1818, aSourceDocShell=0x000000011df841a8, aBaseURI=0x0000000000000000, aDocShell=0x00007fff5fbfb0b8, aRequest=0x000000012c5eb978) + 4784 at nsDocShell.cpp:9564
    frame #52: 0x00000001055c9fce XUL`nsDocShell::OnLinkClickSync(this=0x000000011df84000, aContent=0x000000012c5eb800, aURI=0x0000000125d78680, aTargetSpec=0x000000011e3c05c8, aFileName=0x000000010a1b1818, aPostDataStream=0x000000011e374900, aHeadersDataStream=0x0000000000000000, aDocShell=0x00007fff5fbfb0b8, aRequest=0x000000012c5eb978) + 2654 at nsDocShell.cpp:13255
    frame #53: 0x00000001055ca29b XUL`non-virtual thunk to nsDocShell::OnLinkClickSync(this=0x000000011df84210, aContent=0x000000012c5eb800, aURI=0x0000000125d78680, aTargetSpec=0x000000011e3c05c8, aFileName=0x000000010a1b1818, aPostDataStream=0x000000011e374900, aHeadersDataStream=0x0000000000000000, aDocShell=0x00007fff5fbfb0b8, aRequest=0x000000012c5eb978) + 139 at nsDocShell.cpp:13278
    frame #54: 0x000000010401c598 XUL`mozilla::dom::HTMLFormElement::SubmitSubmission(this=0x000000012c5eb800, aFormSubmission=0x0000000125ed9f60) + 1720 at HTMLFormElement.cpp:828
    frame #55: 0x000000010401bbe8 XUL`mozilla::dom::HTMLFormElement::DoSubmit(this=0x000000012c5eb800, aEvent=0x00007fff5fbfb9b8) + 584 at HTMLFormElement.cpp:686
    frame #56: 0x000000010401a94f XUL`mozilla::dom::HTMLFormElement::DoSubmitOrReset(this=0x000000012c5eb800, aEvent=0x00007fff5fbfb9b8, aMessage=1200) + 191 at HTMLFormElement.cpp:606
    frame #57: 0x000000010401b88e XUL`mozilla::dom::HTMLFormElement::PostHandleEvent(this=0x000000012c5eb800, aVisitor=0x00007fff5fbfb548) + 238 at HTMLFormElement.cpp:559
    frame #58: 0x0000000103f30d21 XUL`mozilla::EventTargetChainItem::PostHandleEvent(this=0x000000012ec5e008, aVisitor=0x00007fff5fbfb548) + 97 at EventDispatcher.cpp:255
    frame #59: 0x0000000103f30fea XUL`mozilla::EventTargetChainItem::HandleEventTargetChain(aChain=0x00007fff5fbfb618, aVisitor=0x00007fff5fbfb548, aCallback=0x0000000000000000, aCd=0x00007fff5fbfb620) + 698 at EventDispatcher.cpp:302
    frame #60: 0x0000000103f312a4 XUL`mozilla::EventTargetChainItem::HandleEventTargetChain(aChain=0x00007fff5fbfb618, aVisitor=0x00007fff5fbfb548, aCallback=0x0000000000000000, aCd=0x00007fff5fbfb620) + 1396 at EventDispatcher.cpp:348
    frame #61: 0x0000000103f32847 XUL`mozilla::EventDispatcher::Dispatch(aTarget=0x000000012c5eb800, aPresContext=0x0000000126553000, aEvent=0x00007fff5fbfb9b8, aDOMEvent=0x0000000000000000, aEventStatus=0x00007fff5fbfb9b4, aCallback=0x0000000000000000, aTargets=0x0000000000000000) + 4935 at EventDispatcher.cpp:633
    frame #62: 0x0000000104f82602 XUL`PresShell::HandleDOMEventWithTarget(this=0x000000012c50f000, aTargetContent=0x000000012c5eb800, aEvent=0x00007fff5fbfb9b8, aStatus=0x00007fff5fbfb9b4) + 162 at nsPresShell.cpp:8421
    frame #63: 0x0000000104045eb6 XUL`mozilla::dom::HTMLInputElement::PostHandleEvent(this=0x000000012d344e60, aVisitor=0x00007fff5fbfc278) + 8262 at HTMLInputElement.cpp:4275
    frame #64: 0x0000000103f30d21 XUL`mozilla::EventTargetChainItem::PostHandleEvent(this=0x000000011a8f3008, aVisitor=0x00007fff5fbfc278) + 97 at EventDispatcher.cpp:255
    frame #65: 0x0000000103f30fea XUL`mozilla::EventTargetChainItem::HandleEventTargetChain(aChain=0x00007fff5fbfc348, aVisitor=0x00007fff5fbfc278, aCallback=0x00007fff5fbfc608, aCd=0x00007fff5fbfc350) + 698 at EventDispatcher.cpp:302
    frame #66: 0x0000000103f312a4 XUL`mozilla::EventTargetChainItem::HandleEventTargetChain(aChain=0x00007fff5fbfc348, aVisitor=0x00007fff5fbfc278, aCallback=0x00007fff5fbfc608, aCd=0x00007fff5fbfc350) + 1396 at EventDispatcher.cpp:348
    frame #67: 0x0000000103f32847 XUL`mozilla::EventDispatcher::Dispatch(aTarget=0x000000012d344e60, aPresContext=0x0000000126553000, aEvent=0x00007fff5fbfca08, aDOMEvent=0x0000000000000000, aEventStatus=0x00007fff5fbfdd18, aCallback=0x00007fff5fbfc608, aTargets=0x0000000000000000) + 4935 at EventDispatcher.cpp:633
    frame #68: 0x0000000104f815d7 XUL`PresShell::HandleEventInternal(this=0x000000012c50f000, aEvent=0x00007fff5fbfca08, aStatus=0x00007fff5fbfdd18) + 4871 at nsPresShell.cpp:8251
    frame #69: 0x0000000104f81a2c XUL`PresShell::HandleEventWithTarget(this=0x000000012c50f000, aEvent=0x00007fff5fbfca08, aFrame=0x00000001335ca338, aContent=0x000000012d344e60, aStatus=0x00007fff5fbfdd18) + 508 at nsPresShell.cpp:7984
    frame #70: 0x0000000103f08d87 XUL`mozilla::EventStateManager::CheckForAndDispatchClick(this=0x000000012e0ffda0, aPresContext=0x0000000126553000, aEvent=0x00007fff5fbfe070, aStatus=0x00007fff5fbfdd18) + 935 at EventStateManager.cpp:4414
    frame #71: 0x0000000103f07349 XUL`mozilla::EventStateManager::PostHandleEvent(this=0x000000012e0ffda0, aPresContext=0x0000000126553000, aEvent=0x00007fff5fbfe070, aTargetFrame=0x00000001335ca338, aStatus=0x00007fff5fbfdd18) + 4985 at EventStateManager.cpp:2924
    frame #72: 0x0000000104f816bb XUL`PresShell::HandleEventInternal(this=0x000000012c50f000, aEvent=0x00007fff5fbfe070, aStatus=0x00007fff5fbfdd18) + 5099 at nsPresShell.cpp:8263
    frame #73: 0x0000000104f802a0 XUL`PresShell::HandlePositionedEvent(this=0x000000012c50f000, aTargetFrame=0x00000001335ca338, aEvent=0x00007fff5fbfe070, aEventStatus=0x00007fff5fbfdd18) + 528 at nsPresShell.cpp:7958
    frame #74: 0x0000000104f7f06e XUL`PresShell::HandleEvent(this=0x000000011c137000, aFrame=0x00000001003ad4e8, aEvent=0x00007fff5fbfe070, aDontRetargetEvents=false, aEventStatus=0x00007fff5fbfdd18) + 8414 at nsPresShell.cpp:7755
    frame #75: 0x0000000104a31e9a XUL`nsViewManager::DispatchEvent(this=0x0000000126513480, aEvent=0x00007fff5fbfe070, aView=0x000000011e3b3fd0, aStatus=0x00007fff5fbfdd18) + 650 at nsViewManager.cpp:775
    frame #76: 0x0000000104a2ec1f XUL`nsView::HandleEvent(this=0x000000011e3b3fd0, aEvent=0x00007fff5fbfe070, aUseAttachedEvents=false) + 319 at nsView.cpp:1097
    frame #77: 0x0000000104a83b05 XUL`nsChildView::DispatchEvent(this=0x000000011c133c00, event=0x00007fff5fbfe070, aStatus=0x00007fff5fbfdeac) + 1221 at nsChildView.mm:1764
    frame #78: 0x0000000104a83bb6 XUL`nsChildView::DispatchWindowEvent(this=0x000000011c133c00, event=0x00007fff5fbfe070) + 54 at nsChildView.mm:1772
    frame #79: 0x0000000104a9437f XUL`-[ChildView mouseUp:](self=0x000000011d3e7ae0, _cmd=0x00007fff969cd122, theEvent=0x000000011e3b67a0) + 463 at nsChildView.mm:4972
    frame #80: 0x00007fff9677902b AppKit`-[NSWindow _reallySendEvent:] + 759
    frame #81: 0x00007fff9620650c AppKit`-[NSWindow sendEvent:] + 368
    frame #82: 0x0000000104adb8cd XUL`-[ToolbarWindow sendEvent:](self=0x000000011c1da540, _cmd=0x00007fff969aa6a9, anEvent=0x000000011e3b67a0) + 365 at nsCocoaWindow.mm:3371
    frame #83: 0x00007fff961b8096 AppKit`-[NSApplication sendEvent:] + 2238
    frame #84: 0x0000000104ac72a2 XUL`-[GeckoNSApplication sendEvent:](self=0x00000001169cc040, _cmd=0x00007fff969aa6a9, anEvent=0x000000011e3b67a0) + 146 at nsAppShell.mm:107
    frame #85: 0x00007fff96044e98 AppKit`-[NSApplication run] + 711
    frame #86: 0x0000000104ac9197 XUL`nsAppShell::Run(this=0x0000000100309ce0) + 167 at nsAppShell.mm:651
    frame #87: 0x00000001059a098c XUL`nsAppStartup::Run(this=0x0000000116e1c740) + 156 at nsAppStartup.cpp:281
    frame #88: 0x0000000105a4eff0 XUL`XREMain::XRE_mainRun(this=0x00007fff5fbfeec8) + 6208 at nsAppRunner.cpp:4150
    frame #89: 0x0000000105a4f89e XUL`XREMain::XRE_main(this=0x00007fff5fbfeec8, argc=5, argv=0x00007fff5fbff7f8, aAppData=0x00007fff5fbff178) + 798 at nsAppRunner.cpp:4226
    frame #90: 0x0000000105a4fd62 XUL`XRE_main(argc=5, argv=0x00007fff5fbff7f8, aAppData=0x00007fff5fbff178, aFlags=0) + 98 at nsAppRunner.cpp:4446
    frame #91: 0x0000000100002d3e firefox`do_main(argc=5, argv=0x00007fff5fbff7f8, xreDirectory=0x000000010030e040) + 1950 at nsBrowserApp.cpp:292
    frame #92: 0x00000001000020a3 firefox`main(argc=5, argv=0x00007fff5fbff7f8) + 323 at nsBrowserApp.cpp:661
    frame #93: 0x0000000100001b04 firefox`start + 52

Basically, the aParent argument to SetDocLoaderParent is not a docshell, so we cannot inherit the PB flag from it.  Later on, we get another call to SetDocLoaderParent which provides an nsDocLoader that is a docshell but it's too late by that time.

Comment 8

[(no longer active)]

Attachment: WIP for bug 1108547

Comment 9

[Al Billings [:abillings - ex-MoCo]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #5)
> I don't think this is a security bug.  Is it OK to open it up?

Minusing this for bounty and removing rating based on this.

Comment 10

[(no longer active)]

Josh: ping?

Comment 11

[Josh Matthews [:jdm]]

Ehsan's working on this, and we discussed his question that wasn't left here as a comment.

Comment 12

[(no longer active)]

I talked to Josh about this in person.  The best idea we came up with was to extend nsDocShell::SetDocLoaderParent with a second argument (nsDocLoader* aSettingsParent or some such) will has to be non-null when calling this through nsDocShell::Init.  nsDocShell::Init itself of course has no idea what the correct settings parent should be, but nsFrameLoader::MaybeCreateDocShell can supply that information through mOwnerContent.

Comment 13

[(no longer active)]

(In reply to :Ehsan Akhgari [Away: 1/29-2/20] (not reading bugmail, needinfo? me!) from comment #12)
> I talked to Josh about this in person.  The best idea we came up with was to
> extend nsDocShell::SetDocLoaderParent with a second argument (nsDocLoader*
> aSettingsParent or some such) will has to be non-null when calling this
> through nsDocShell::Init.  nsDocShell::Init itself of course has no idea
> what the correct settings parent should be, but
> nsFrameLoader::MaybeCreateDocShell can supply that information through
> mOwnerContent.

That was all a huge red herring.  :(

The issue is that nsHTMLDocument::GetCookie calls nsCookieService::GetCookieString with a null channel, and this check <https://dxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#1620> will happily think that we are in a non-private window.  Basically, without a channel, the cookie service doesn't know that we're requesting a private cookie.  Whereas without target="_blank", we end up running the javascript: URI code within the context of the existing tab, where nsDocument::mChannel points to an existing channel which has the correct privacy bit set.

The only way that I can think of to fix this is to extend nsICookieService::GetCookieString with a privacy flag and pass the right thing from the document.

Comment 14

[(no longer active)]

Except that juggling a boolean flag with the privacy status of the channel really sucks.  I'm thinking perhaps only honoring aIsPrivate when the channel is null?  Josh, any better ideas?

Comment 15

[Josh Matthews [:jdm]]

What about creating a dummy channel (if mChannel is null) and calling channel.setPrivate on it? I mean, it's icky, but...

Comment 16

[(no longer active)]

(In reply to Josh Matthews [:jdm] from comment #15)
> What about creating a dummy channel (if mChannel is null) and calling
> channel.setPrivate on it? I mean, it's icky, but...

That idea makes me sick to my stomach.  :(  But I'll try it, I guess it's better than messing with the cookie service.

Comment 17

[(no longer active)]

Attachment: Part 1: Inherit the correct private browsing status on new windows created for targeted navigations

Comment 18

[(no longer active)]

Attachment: Part 2: Create a dummy channel for accessing the document's cookie when we don't have one already

This ensures that the cookie service can know which cookie database
to query from.  This is a gross hack, please see the discussion on
the bug as to why we did this.

Comment 19

[(no longer active)]

Attachment: Part 3: Automated tests

Comment 20

[(no longer active)]

Comment on attachment 8552835 [details] [diff] [review]
Part 3: Automated tests

Oops, a couple of typos caused this to leak two windows until shutdown.

Comment 21

[(no longer active)]

Attachment: Part 3: Automated tests

Comment 22

[Josh Matthews [:jdm]]

Comment on attachment 8552838 [details] [diff] [review]
Part 3: Automated tests

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

::: dom/html/test/browser_bug1108547.js
@@ +62,5 @@
> +    privateWin.gBrowser.tabs[privateWin.gBrowser.tabs.length - 1].linkedBrowser.addEventListener("load", onNewTabLoaded, true);
> +  }
> +
> +  function onNewTabLoaded() {
> +    privateWin.gBrowser.selectedBrowser.removeEventListener("load", onNewTabLoaded, true);

Is selectedBrowser the right one here? Should we use it for adding the listener too?

@@ +104,5 @@
> +    gBrowser.tabs[gBrowser.tabs.length - 1].linkedBrowser.addEventListener("load", onNewTabLoaded2, true);
> +  }
> +
> +  function onNewTabLoaded2() {
> +    gBrowser.selectedBrowser.removeEventListener("load", onNewTabLoaded2, true);

Same questions as previously.

Comment 23

[(no longer active)]

Comment on attachment 8552838 [details] [diff] [review]
Part 3: Automated tests

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

::: dom/html/test/browser_bug1108547.js
@@ +62,5 @@
> +    privateWin.gBrowser.tabs[privateWin.gBrowser.tabs.length - 1].linkedBrowser.addEventListener("load", onNewTabLoaded, true);
> +  }
> +
> +  function onNewTabLoaded() {
> +    privateWin.gBrowser.selectedBrowser.removeEventListener("load", onNewTabLoaded, true);

Hmm, yeah perhaps that is unreliable...  I'll fix it to use privateWin.gBrowser.tabs[privateWin.gBrowser.tabs.length - 1].linkedBrowser.

Comment 24

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/211567afcc5f
https://hg.mozilla.org/integration/mozilla-inbound/rev/91101cbcc319
https://hg.mozilla.org/integration/mozilla-inbound/rev/180e7905987e

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Backed out for timeouts in the new test.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f06c4beef24

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=5916536&repo=mozilla-inbound

Comment 26

[(no longer active)]

Note to self: <https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=180e7905987e>

Comment 27

[(no longer active)]

The failure is Linux specific.

Comment 28

[Josh Matthews [:jdm]]

>TEST-UNEXPECTED-PASS | /html/dom/documents/resource-metadata-management/document-cookie.html | getting cookie for a cookie-averse document returns empty string, setting does nothing - expected FAIL

That looks fun, too :(

Comment 29

[(no longer active)]

The Linux test failures were caused because of whenWindowLoaded.  Just waiting for the delayed startup event should be enough.

Comment 30

[(no longer active)]

(In reply to Josh Matthews [:jdm] from comment #28)
> >TEST-UNEXPECTED-PASS | /html/dom/documents/resource-metadata-management/document-cookie.html | getting cookie for a cookie-averse document returns empty string, setting does nothing - expected FAIL
> 
> That looks fun, too :(

According to the spec, these kinds of documents are cookie averse, and should not accept a cookie.  With my patch, CreateDummyChannelForCookies returns null since aCodebaseURI is null, which makes the SetCookie function fail.  I will remove the expected failure annotation from this test.

Comment 31

[(no longer active)]

(In reply to :Ehsan Akhgari [Away: 1/29-2/20] (not reading bugmail, needinfo? me!) from comment #30)
> (In reply to Josh Matthews [:jdm] from comment #28)
> > >TEST-UNEXPECTED-PASS | /html/dom/documents/resource-metadata-management/document-cookie.html | getting cookie for a cookie-averse document returns empty string, setting does nothing - expected FAIL
> > 
> > That looks fun, too :(
> 
> According to the spec, these kinds of documents are cookie averse, and
> should not accept a cookie.  With my patch, CreateDummyChannelForCookies
> returns null since aCodebaseURI is null, which makes the SetCookie function
> fail.  I will remove the expected failure annotation from this test.

Does this sound good?

Comment 32

[Josh Matthews [:jdm]]

Oh heh, I didn't notice the unexpected-pass bit. Yep!

Comment 33

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/edbdf7aa292c
https://hg.mozilla.org/integration/mozilla-inbound/rev/70f043c2e6c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/a70ae54bd318

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/edbdf7aa292c
https://hg.mozilla.org/mozilla-central/rev/70f043c2e6c8
https://hg.mozilla.org/mozilla-central/rev/a70ae54bd318