Closed Bug 1250063 Opened 8 years ago Closed 8 years ago

mContentViewer shouldn't load any document except for about:blank when setting origin attributes on the docshell

Categories

(Core :: Security, defect, P1)

Product:
Core
Component:
Security
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Iteration:
49.2 - May 23
Tracking Flags:
Tracking Status
firefox47 --- affected
firefox49 --- fixed

People

(Reporter: allstars.chh, Assigned: allstars.chh)

References

(Blocks 1 open bug)

Details

(Whiteboard: [OA])

Attachments

(3 files, 6 obsolete files)

Part 1: Pass userContextId in duplicateTab. v2
5.06 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
Part 2: don't call notifyTabContextUpdated() when swapFrameLoader v2
3.89 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
Part 3: mContentViewer shouldn't load any document except for about:blank when setting origin attributes on docshell.
1.43 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review 
Hi Jonas
So far when calling setOriginAttributes on a docshell, if we assert on the mContentViewer(it could only load about:blank"), some tests from sessionstore will crash.

And the reason for the crash is, those tests will first try to load a test html, then they will try to close the tab and restore it. When doing restore, SessionHistory.jsm will set the userContextId (from Bug 1193854), and now since the mContentViewer has loaded the document from the test html, so those tests will crash.

I am not quite familiar with SessionStore, but can you give me some suggestions here?
It looks to me that somehow we should reset the mContentViewer at somewhere.

Thanks
Flags: needinfo?(jonas)
We should not create a new mContentViewer.

The point is that we should make sure to call SetOriginAttributes before we get to the point when we start loading documents into the docshell. If we load documents before calling SetOriginAttributes, that means that we'll use the wrong cookie jar when loading the document. Trying to call SetOriginAttributes won't fix that.

It sounds like the SessionStore code needs to be updated. If I understand you correctly, you are saying that it loads (or restores) a document into a tab, and *then* calls SetOriginAttributes on the same tab? If so, we need to update it so that it first calls SetOriginAttributes on the tab, and then loads a document into it.

Possibly the way to archive this is that when the SessionStore code sees that it should restore a tab which has a non-default userContextId (or other non-default OriginAttribute) it should create a new tab rather than load into an existing tab. Note that this does not need to affect the resulting UI.

I'd recommend talking to Kate McKinley who has worked on userContextIds.
Flags: needinfo?(jonas)
Attached patch Patch. (obsolete) — — Splinter Review 
simply add a check for userContextId and avoid the crashes from sessionstore.

This patch is base on Bug 1227861, as the original code from Bug 1239420 will call childDocShell->setUserContextId when appendChild, which will cause more crashes, and Bug 1227861 will remove that line.

Will ask for r? when Bug 1227861 gets r+.
Summary: mContentViewer should be null when setting origin attributes on docshell → mContentViewer shouldn't load any document except for "about:blank" when setting origin attributes on the docshell
Comment on attachment 8723997 [details] [diff] [review]
Patch.

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

Bug 1227861 already got r+, ask feedback? from baku for the SessionHistory.jsm change since that's from your Bug 1193854.
Thanks
Attachment #8723997 - Flags: feedback?(amarchesini)
Comment on attachment 8723997 [details] [diff] [review]
Patch.

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

::: browser/components/sessionstore/SessionHistory.jsm
@@ +260,3 @@
>        let attrs = docShell.getOriginAttributes();
>        attrs.userContextId = tabData.userContextId;
>        docShell.setOriginAttributes(attrs);

Why do you want to set this attribute only when it's different than 0?
It should work also in this case, correct? I don't think this change is useful, but tell me more.

::: docshell/base/nsDocShell.cpp
@@ +13944,5 @@
>  {
> +  if (mContentViewer) {
> +   nsIDocument* doc = mContentViewer->GetDocument();
> +   if (doc) {
> +     nsIURI* uri = doc->GetDocumentURI();

This can be null.

@@ +13946,5 @@
> +   nsIDocument* doc = mContentViewer->GetDocument();
> +   if (doc) {
> +     nsIURI* uri = doc->GetDocumentURI();
> +     nsAutoCString uriSpec;
> +     uri->GetSpec(uriSpec);

This can return an error. You should handle it.
Attachment #8723997 - Flags: feedback?(amarchesini) → feedback-
Comment on attachment 8723997 [details] [diff] [review]
Patch.

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

::: browser/components/sessionstore/SessionHistory.jsm
@@ +260,3 @@
>        let attrs = docShell.getOriginAttributes();
>        attrs.userContextId = tabData.userContextId;
>        docShell.setOriginAttributes(attrs);

With the assertion in nsDocShell::SetOriginAttributes, some tests will crash, see comment 0.

And the reason is from comment 1,

for example, some test will first load foo.html, then it will call some APIs in nsISessionStore.idl to restore tab/window state, then the call will in turn call SessionHistory.restore here, at this time docShell has loaded foo.html, therefore it will cause the assertion to crash.

perhaps we should do some refactoring here, like 
- clear mContentViewer somewhere in Session restore, 
- or we shouldn't call setOriginAttributes here as docShell could already load some documen
- or create a tab as Jonas said in comment 2,
but I am not quite sure.

So far the tests will crash are old bugs, like tests from Bug 423132, Bug 491168.
And given xul::browser will set userContextId 0 here, so I just simply add the check.

::: docshell/base/nsDocShell.cpp
@@ +13946,5 @@
> +   nsIDocument* doc = mContentViewer->GetDocument();
> +   if (doc) {
> +     nsIURI* uri = doc->GetDocumentURI();
> +     nsAutoCString uriSpec;
> +     uri->GetSpec(uriSpec);

I'll check result in my next patch.

Thanks
Comment on attachment 8723997 [details] [diff] [review]
Patch.

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

::: docshell/base/nsDocShell.cpp
@@ +13946,5 @@
> +   nsIDocument* doc = mContentViewer->GetDocument();
> +   if (doc) {
> +     nsIURI* uri = doc->GetDocumentURI();
> +     nsAutoCString uriSpec;
> +     uri->GetSpec(uriSpec);

If it returns an error then we know it's not a about:blank URI and so we should assert. Which we do. So I think that this part actually works.

You still need the nullcheck about though.


That said, can we really not assert that mContentViewer is null? In what cases do we end up with a non-null contentviewer? Can we change the code there so that SetOriginAttributes is called earlier?
Attached patch Patch v2. (obsolete) — — Splinter Review 
I realized that after Bug 1227861 is landed, when a docshell is created it will already have the origin attributes ready (set by nsFrameLoader::MaybeCreateDocShell).

So we don't have to set OA in SessionHistory.jsm again.
Attachment #8723997 - Attachment is obsolete: true
Attachment #8727267 - Flags: review?(jonas)
Attachment #8727267 - Flags: feedback?(amarchesini)
Comment on attachment 8727267 [details] [diff] [review]
Patch v2.

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

This looks good to me, but I don't know very well how the session-restore code interacts with userContextIds. So someone that knows that better than me should review this. I suspect Kate does.
Attachment #8727267 - Flags: review?(kmckinley)
Attachment #8727267 - Flags: review?(jonas)
Attachment #8727267 - Flags: feedback+
Summary: mContentViewer shouldn't load any document except for "about:blank" when setting origin attributes on the docshell → mContentViewer should be null when setting origin attributes on the docshell
I found out the patch has some failures on Android, fixing this now.
Comment on attachment 8727267 [details] [diff] [review]
Patch v2.

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

More than Kate, we should ask ttaubert, who reviewed my code here.
Attachment #8727267 - Flags: review?(kmckinley) → review?(ttaubert)
Comment on attachment 8727267 [details] [diff] [review]
Patch v2.

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

More than Kate, we should ask ttaubert, who reviewed my code here.

::: browser/components/sessionstore/SessionHistory.jsm
@@ -265,5 @@
> -
> -    if ("userContextId" in tabData) {
> -      let attrs = docShell.getOriginAttributes();
> -      attrs.userContextId = tabData.userContextId;
> -      docShell.setOriginAttributes(attrs);

If you remove this block, where do we set the userContextId in this new tab?
Attachment #8727267 - Flags: feedback?(amarchesini)
(In reply to Andrea Marchesini (:baku) from comment #14)
> ::: browser/components/sessionstore/SessionHistory.jsm
> @@ -265,5 @@
> > -
> > -    if ("userContextId" in tabData) {
> > -      let attrs = docShell.getOriginAttributes();
> > -      attrs.userContextId = tabData.userContextId;
> > -      docShell.setOriginAttributes(attrs);
> 
> If you remove this block, where do we set the userContextId in this new tab?

As explained briefly in Comment 8, when a new tab is created, a new docshell will be also created from nsFrameLoader::MaybeCreateDocShell, and the top level docshell will get the user context id from XUL, 

https://dxr.mozilla.org/mozilla-central/source/dom/base/nsFrameLoader.cpp?from=nsFrameLoader.cpp#1962

and the child docshells will inherit user context id from its parent document 
(will have more detail in bug 1252800)
https://dxr.mozilla.org/mozilla-central/source/dom/base/nsFrameLoader.cpp?from=nsFrameLoader.cpp#1923
Flags: needinfo?(amarchesini)
Will we set the usercontextid attribute on the <xul:browser> as we should? Otherwise the docshell in the restored tab will not get the correct OA.userContextId
I just tried the patch and it crashes at startup:

Assertion failure: !mContentViewer, at /home/baku/Sources/m/aaa/src/docshell/base/nsDocShell.cpp:13995

(gdb) bt
#0  0x00007fffe74fdae0 in nsDocShell::SetOriginAttributes (this=0x153d810, aAttrs=...) at /home/baku/Sources/m/aaa/src/docshell/base/nsDocShell.cpp:13995
#1  0x00007fffe752a2e9 in nsWindowWatcher::OpenWindowInternal (this=0xc126c0, aParent=0x0, aUrl=0x1018830 "chrome://mozapps/content/extensions/update.xul", aName=0x10186c0 "", 
    aFeatures=0x1018870 "chrome,centerscreen,dialog,titlebar,modal", aCalledFromJS=false, aDialog=true, aNavigate=true, aOpeningTab=0x0, aArgv=0x10188e0, aResult=0x7fffffff8960)
    at /home/baku/Sources/m/aaa/src/embedding/components/windowwatcher/nsWindowWatcher.cpp:1027
#2  0x00007fffe7527d95 in nsWindowWatcher::OpenWindow (this=0xc126c0, aParent=0x0, aUrl=0x1018830 "chrome://mozapps/content/extensions/update.xul", aName=0x10186c0 "", 
    aFeatures=0x1018870 "chrome,centerscreen,dialog,titlebar,modal", aArguments=0x100b790, aResult=0x7fffffff8960) at /home/baku/Sources/m/aaa/src/embedding/components/windowwatcher/nsWindowWatcher.cpp:371
#3  0x00007fffe31cc77d in NS_InvokeByIndex (that=0xc126c0, methodIndex=3, paramCount=6, params=0x7fffffff88e8) at /home/baku/Sources/m/aaa/src/xpcom/reflect/xptcall/md/unix/xptcinvoke_x86_64_unix.cpp:182
#4  0x00007fffe407668d in CallMethodHelper::Invoke (this=0x7fffffff88a0) at /home/baku/Sources/m/aaa/src/js/xpconnect/src/XPCWrappedNative.cpp:2083
#5  0x00007fffe40740aa in CallMethodHelper::Call (this=0x7fffffff88a0) at /home/baku/Sources/m/aaa/src/js/xpconnect/src/XPCWrappedNative.cpp:1400
#6  0x00007fffe4056f10 in XPCWrappedNative::CallMethod (ccx=..., mode=XPCWrappedNative::CALL_METHOD) at /home/baku/Sources/m/aaa/src/js/xpconnect/src/XPCWrappedNative.cpp:1367
#7  0x00007fffe4060589 in XPC_WN_CallMethod (cx=0x544410, argc=5, vp=0xaf8368) at /home/baku/Sources/m/aaa/src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1115
#8  0x00007fffe91b5eae in js::CallJSNative (cx=0x544410, native=0x7fffe406023c <XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)>, args=...) at /home/baku/Sources/m/aaa/src/js/src/jscntxtinlines.h:235
#9  0x00007fffe918a698 in js::Invoke (cx=0x544410, args=..., construct=js::NO_CONSTRUCT) at /home/baku/Sources/m/aaa/src/js/src/vm/Interpreter.cpp:478
#10 0x00007fffe9199f6d in Interpret (cx=0x544410, state=...) at /home/baku/Sources/m/aaa/src/js/src/vm/Interpreter.cpp:2802
#11 0x00007fffe918a2f1 in js::RunScript (cx=0x544410, state=...) at /home/baku/Sources/m/aaa/src/js/src/vm/Interpreter.cpp:428
#12 0x00007fffe918a758 in js::Invoke (cx=0x544410, args=..., construct=js::NO_CONSTRUCT) at /home/baku/Sources/m/aaa/src/js/src/vm/Interpreter.cpp:496
#13 0x00007fffe8f3a7e1 in js::fun_apply (cx=0x544410, argc=2, vp=0x7fffffff9ca8) at /home/baku/Sources/m/aaa/src/js/src/jsfun.cpp:1277
#14 0x00007fffe91b5eae in js::CallJSNative (cx=0x544410, native=0x7fffe8f3a24b <js::fun_apply(JSContext*, unsigned int, JS::Value*)>, args=...) at /home/baku/Sources/m/aaa/src/js/src/jscntxtinlines.h:235
#15 0x00007fffe918a698 in js::Invoke (cx=0x544410, args=..., construct=js::NO_CONSTRUCT) at /home/baku/Sources/m/aaa/src/js/src/vm/Interpreter.cpp:478
#16 0x00007fffe918aa58 in js::Invoke (cx=0x544410, thisv=..., fval=..., argc=2, argv=0xaf81c8, rval=...) at /home/baku/Sources/m/aaa/src/js/src/vm/Interpreter.cpp:530
#17 0x00007fffe904f0f9 in js::DirectProxyHandler::call (this=0x7fffed071b70 <js::CrossCompartmentWrapper::singleton>, cx=0x544410, proxy=..., args=...) at /home/baku/Sources/m/aaa/src/js/src/proxy/DirectProxyHandler.cpp:77
#18 0x00007fffe904c899 in js::CrossCompartmentWrapper::call (this=0x7fffed071b70 <js::CrossCompartmentWrapper::singleton>, cx=0x544410, wrapper=..., args=...)
    at /home/baku/Sources/m/aaa/src/js/src/proxy/CrossCompartmentWrapper.cpp:289
#19 0x00007fffe905726a in js::Proxy::call (cx=0x544410, proxy=..., args=...) at /home/baku/Sources/m/aaa/src/js/src/proxy/Proxy.cpp:391

I wanted to see if the restore procedure works correctly with containers, but I cannot start FF with an existing profile.
It crashes because 1 patch I landed recently: bug 1249224. So maybe also that code has to be changed.
Flags: needinfo?(amarchesini)
Flags: needinfo?(allstars.chh)
(In reply to Andrea Marchesini (:baku) from comment #17)
> I wanted to see if the restore procedure works correctly with containers,
> but I cannot start FF with an existing profile.
> It crashes because 1 patch I landed recently: bug 1249224. So maybe also
> that code has to be changed.
Yes, I notice your bug 1249224 this morning and is working on it now.
I'll upload another part to fix it, as it may require different reviewer from session store.

If you have time, you could revert your bug 1249224 to check this.
As I have run try in Comment 9, it passes your test from Bug 1193854.

But I'd like to get feedback from you in case I break something :(

Thanks
Flags: needinfo?(allstars.chh)
> Yes, I notice your bug 1249224 this morning and is working on it now.
> I'll upload another part to fix it, as it may require different reviewer
> from session store.

About session store, I confirm that your patch works. But still I cannot revert my changes from m-i because it fixes a window.open() bug when used from a container tab.
(In reply to Andrea Marchesini (:baku) from comment #19)
> About session store, I confirm that your patch works. But still I cannot
> revert my changes from m-i because it fixes a window.open() bug when used
> from a container tab.
Agree, I'll fix the assert and possibly will need your feedback again :)
Thanks
So far there are two places will trigger the assert, and both of them mContentViewer has already loaded "about:blank"

1. TabChild::NotifyTabContextUpdated
its docshell will be created and load about:blank from nsWebBrowser::Create, so when NotifyTabContextUpdated() is called in TabChild::Init, it has already loaded about:blank.

2. nsWindowWatcher::OpenWindowInternal
The docshell will be created and load about:blank from nsWebShellWindow::Initialize, then OpenWindowInternal will set UserContextId on it (bug 1249224)

Will spend more time on checking this.
Comment on attachment 8727267 [details] [diff] [review]
Patch v2.

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

::: browser/components/sessionstore/SessionHistory.jsm
@@ -265,5 @@
> -
> -    if ("userContextId" in tabData) {
> -      let attrs = docShell.getOriginAttributes();
> -      attrs.userContextId = tabData.userContextId;
> -      docShell.setOriginAttributes(attrs);

Andreas is right here. Your patch sets a userContextId only when creating new tabs, which happens when starting Firefox. SessionStore can override (restore into) existing tabs though. Jonas mentioned that and it seems to be tricky to change the userContextId for an existing tab?

FWIW, the change to SessionStore.jsm seems fine but I don't understand the original issue, SessionHistory.restore() is called before we kick off any document loads.

What we do though is restore into existing tabs. Maybe that's the actual problem you're hitting here.
Attachment #8727267 - Flags: review?(ttaubert)
(In reply to Tim Taubert [:ttaubert] from comment #23)
> Andreas is right here.

I meant Andrea, of course. Sorry for the typo :|
Comment on attachment 8727267 [details] [diff] [review]
Patch v2.

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

This patch works and it makes my code simpler. So f+.
Attachment #8727267 - Flags: feedback+
(In reply to Tim Taubert [:ttaubert] from comment #23)
> SessionStore can override
> (restore into) existing tabs though. Jonas mentioned that and it seems to be
> tricky to change the userContextId for an existing tab?

We must never restore into an existing tab unless the userContextId of the restored page matches the userContextId of the tab we are restoring into.

If the two don't match, we need to create a new tab with the correct userContextId and restore into that, or we could grab another tab which has a matching userContextId.


To put it another way: It is impossible to *safely* change the userContextId of an existing tab, so if we restore into a tab that doesn't have a correct userContextId, we'll end up with security problems somewhere.
(In reply to Jonas Sicking (:sicking) from comment #26)
> (In reply to Tim Taubert [:ttaubert] from comment #23)
> > SessionStore can override
> > (restore into) existing tabs though. Jonas mentioned that and it seems to be
> > tricky to change the userContextId for an existing tab?
> 
> We must never restore into an existing tab unless the userContextId of the
> restored page matches the userContextId of the tab we are restoring into.

Ok, so that's a new constraint sessionstore isn't aware of yet.

We do have a few ways to reuse existing tabs, one of those is ss.setTabState(tab, state). If we would simply remove |tab| and add a new one in its place that's probably going to confuse add-ons, tests, and maybe other consumers as to why the tab/browser reference isn't valid anymore.

Would it be possible to have the frameLoader discard the current docShell and create a new one when origin attributes are modified? Another solution might be to use the same approach that process switching uses when loading a new URL in a different process, i.e. replace the underlying <xul:browser>.
I think trying to reuse an existing tab is going to create a lot of complexity in the frontend as well as in the backend.

* Will addons and parts of our frontend that already have a reference to the given tab be aware that
  its userContextId has switched?

* What happens if the user press the "back" button? Does that mean that we should navigate back to the
  old page and at that point switch the userContextId to its old value?


Could you explain in more detail when sessionrestore reuse an existing tab?

Would it be possible to make "sessionrestore kills an existing tab and creates a new one" look like "user closes an existing tab and opens a new one" towards addons?
(In reply to Jonas Sicking (:sicking) from comment #16)
> Will we set the usercontextid attribute on the <xul:browser> as we should?
> Otherwise the docshell in the restored tab will not get the correct
> OA.userContextId

Yes, in https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1742

(In reply to Tim Taubert [:ttaubert] from comment #23)
> ::: browser/components/sessionstore/SessionHistory.jsm
> @@ -265,5 @@
> > -
> > -    if ("userContextId" in tabData) {
> > -      let attrs = docShell.getOriginAttributes();
> > -      attrs.userContextId = tabData.userContextId;
> > -      docShell.setOriginAttributes(attrs);
> 
> 
> FWIW, the change to SessionStore.jsm seems fine but I don't understand the
> original issue, SessionHistory.restore() is called before we kick off any
> document loads.
When gBrowser.addTab() is called, it will create a docshell and load "about:blank"(called by nsDocShell::CreateAboutBlankContentViewer, which in turn called by some JS code or frameScript from my gdb stack), so when we call SessionHistory.restore, setting userContextId will trigger the assertion.

As my Bug 1227861 will set the userContextId when the docshell is created by nsFrameLoader::MaybeCreateDocShell, so I remove the code from SessionHistory.jsm

> 
> What we do though is restore into existing tabs. Maybe that's the actual
> problem you're hitting here.
Since Jonas already has some comments for this, I'll wait for comments from you two.
ni? for ttaubert for Jonas' question Comment 28, also my comment for your previous review in Comment 29.

Thanks
Flags: needinfo?(ttaubert)
Hi Jonas
Besides SessionRestore, I have some questions for other components regarding to docshell.
Previous in nsFrameLoader we inherit attributes from doc of parent docshell.
But for other components, where should we inherit attributes from?

There are two other places will create docshell.
1. nsWebShellWindow:
http://lxr.mozilla.org/mozilla-central/source/xpfe/appshell/nsWebShellWindow.cpp#186

Should the docshell inherit attributes from subjectPrincipal?
http://lxr.mozilla.org/mozilla-central/source/xpfe/appshell/nsWebShellWindow.cpp#218

2. nsWebBrowser.cpp
http://lxr.mozilla.org/mozilla-central/source/embedding/browser/nsWebBrowser.cpp#1186
This will be called by TabChild::Init
But I am not sure where we should do the inhertance from?

Thanks
Flags: needinfo?(jonas)
> 1. nsWebShellWindow:
> http://lxr.mozilla.org/mozilla-central/source/xpfe/appshell/nsWebShellWindow.
> cpp#186

No idea :( I don't know this code at all, or when/how it is used. Maybe try asking smaug.

> 2. nsWebBrowser.cpp
> http://lxr.mozilla.org/mozilla-central/source/embedding/browser/nsWebBrowser.
> cpp#1186
> This will be called by TabChild::Init
> But I am not sure where we should do the inhertance from?

Is this different from the code that runs when we have an <iframe> or <xul:browser> in the parent?
Flags: needinfo?(jonas)
Hi Bz
Please refer to Jonas' comment in https://bugzilla.mozilla.org/show_bug.cgi?id=1227861#c53
and Comment 7 for my purpose 

Right now I try to add assert for !mContentViewer when calling nsDocShell::SetOriginAttributes, it will crash since Bug 1249224 calls SetOriginAttributs() in 
http://lxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/nsWindowWatcher.cpp#1027

and after reading your comment https://bugzilla.mozilla.org/show_bug.cgi?id=1249224#c9
My question is can we set the origin attributes inside nsWebShellWindow::Initialize,
for in that function a docshell is created and loads about:blank.

http://lxr.mozilla.org/mozilla-central/source/xpfe/appshell/nsWebShellWindow.cpp#186
http://lxr.mozilla.org/mozilla-central/source/xpfe/appshell/nsWebShellWindow.cpp#218

For example, can we do
mDocShell->SetOriginAttributes(BasePrincipal::Cast(subjectPrincipal)->OriginAttributesRef()); 
before loading about:blank inside nsWebShellWindow::Initialize?

However in nsWebShellWindow::Initialize we don't have information for the windowIsNew.
But I am not familar with the code here, 
so would like to get some comments from you.

Thanks
Flags: needinfo?(bzbarsky)
I have no idea, sorry.  I'd have to dig around for a while sorting out how we even end up in nsWebShellWindow and so forth.  That said, windowIsNew is true in nsWebShellWindow::Initialize pretty much by definition, I would think.

The only other useful thought I have is that per spec, the initial about:blank is created together with the browsing context.  Which means that long-term we want to move to a world where we create the initial about:blank in the docshell's Create() method.  Whatever information is needed to do it properly should be passed into there.
Flags: needinfo?(bzbarsky)
(In reply to Jonas Sicking (:sicking) from comment #28)
> * Will addons and parts of our frontend that already have a reference to the
> given tab be aware that
>   its userContextId has switched?

Depends on how we do that. If we simply reuse that tab they'd probably only find out through an attribute listener. With process switching we simply replace the underlying <xul:browser> and it looks like that doesn't pose a huge problem.

> * What happens if the user press the "back" button? Does that mean that we
> should navigate back to the
>   old page and at that point switch the userContextId to its old value?

Restoring into a tab replaces its whole state, including the session history. Going back is possible if the new state has more than one shistory entry, but that would then be the same userContextId.

> Could you explain in more detail when sessionrestore reuse an existing tab?

There are ss.setWindowState() and ss.setTabState() methods, as well as ss.setBrowserState(). Depending on the old and the new state we will simply reuse tabs so we don't have to close them if they're already there. Those APIs are mostly used by session management add-ons, but maybe not exclusively.

> Would it be possible to make "sessionrestore kills an existing tab and
> creates a new one" look like "user closes an existing tab and opens a new
> one" towards addons?

Yeah, gBrowser.removeTab() is what we use today already, when the new window state has less tabs than the old state. We can close and open tabs immediately without animation, the user probably won't see a difference to what we're doing now. Add-ons receive close and open notifications as usual and can't differentiate whether that was a programmatic or user action.
Flags: needinfo?(ttaubert)
Another important question, probably for Kate or Andrea:

What is the UX for choosing to use the non-default userContextId? Does the user right-click on an existing tab and use the context menu for choosing which userContextId to use? Or do we somewhere have UI for "open a new tab with userContextId X"?

If the user can switch userContextId of a tab, how do we implement that? Do we keep the existing tab, but create a new <xul:browser> for that tab? Or do we close the tab completely and create a new tab with a different userContextId? Or do we simply not have UX for switching userContextId and instead we only have UI for "open a new tab with userContextId X"?
Generally speaking, I don't think I'm providing much value here. Between Tim, Kate, Andrea and Yoshi I think you guys have enough information to come up with a solution.

The platform constraints are simply:
* We can never change OriginAttributes of an existing document. This is equivalent to changing origin
  of an existing document which is just not possible.
* The userContextId of a document should never be out-of-sync with the userContextId of the docshell the
  document is in.
* Thus we should never change userContextId of docshell which has ever opened a document.
* Thus we should never change the userContextId attribute of a <xul:browser> which we've ever loaded a
  document into.
* I'm uncertain of if it's ok to change <xul:browser> of an existing tab or not. From a platform point of
  view that is *probably* ok as far as I know (others probably know better), but I don't know if the
  frontend code would have problems with this.
* Adding userContextIds to Firefox is a big change. We might need to change a few APIs to do it, thus
  maybe breaking a few addons.
(In reply to Jonas Sicking (:sicking) from comment #39)
> Another important question, probably for Kate or Andrea:
> 
> What is the UX for choosing to use the non-default userContextId? Does the
> user right-click on an existing tab and use the context menu for choosing
> which userContextId to use? Or do we somewhere have UI for "open a new tab
> with userContextId X"?
I just tried nightly with privacy.userContext.enabled enabled
The UX to choose userContextId is 
File -> New Container Tab -> choose one of 'Personal', 'Work', 'Banking' or 'Shopping'
Right-click on an existing tab or press the '+' button cannot choose userContextId

> 
> If the user can switch userContextId of a tab, how do we implement that?
Kate has replied in https://bugzilla.mozilla.org/show_bug.cgi?id=1227861#c26 before

> Do
> we keep the existing tab, but create a new <xul:browser> for that tab? Or do
> we close the tab completely and create a new tab with a different
> userContextId? Or do we simply not have UX for switching userContextId and
> instead we only have UI for "open a new tab with userContextId X"?
Ok, based on that I propose that when restoring a session, if we're in a situation where we previously would have reused an existing tab, and if the userContextId of that tab does *not* match the userContextId of the restore information, then call gBrowser.removeTab() to remove the tab and then create a new tab, rather than restore into the existing tab.

Otherwise use existing logic for when to restore into existing tabs and when to create new tabs.
Priority: -- → P1
Whiteboard: [OA]
Status: NEW → ASSIGNED
Iteration: --- → 49.2 - May 23
Comment on attachment 8729425 [details] [diff] [review]
Part 1: don't set userContextId in SessionRestore

Hi Tim
Sorry for asking another r? in such a long time. But I do think this is the correct way to do this, after your comment 23 I thought I have to fix 'restoring into an existing tab with different userContextId' first, so I filed Bug 1257453 and Bug 1257456, but in the end I think they are seperate problems than this bug, so I decide to fix this one first.

So far the problem in Gecko is in most tests they are not related to userContextId, however they will use some BrowserTestUtils API (for example, promiseTabRestored) or SessionStore API, and in the end, SessionHistory will call docShell.setOriginAttributes, which is too late because the docshell has already loaded document, if we change origin attributes this time, which will cause some security problem because that will affect the principal of document in turn.

I have another test, to assert that the userContextId of the docshell is matched with tabData.userContext in SessionHistory.restoreHistory.
https://hg.mozilla.org/try/rev/aa7ee8fd6f693e5c756761edf46c3f6b845de3ba
try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c3a7b509ab6
should prove my point.

(In reply to Tim Taubert [:ttaubert] from comment #23)
> ::: browser/components/sessionstore/SessionHistory.jsm
> @@ -265,5 @@
> > -
> > -    if ("userContextId" in tabData) {
> > -      let attrs = docShell.getOriginAttributes();
> > -      attrs.userContextId = tabData.userContextId;
> > -      docShell.setOriginAttributes(attrs);
> 
> Andreas is right here. Your patch sets a userContextId only when creating
> new tabs, which happens when starting Firefox.
that happens when a tab is created, for example, gBrowser.addTab() is called.
gBrowser.addTab will call tab.setAttribute("usercontextid", userContextId); on the tab, then when nsFrameLoader::MaybeCreateDocShell is called, it will inherit the usercontextid value from the tab.

> SessionStore can override
> (restore into) existing tabs though. Jonas mentioned that and it seems to be
> tricky to change the userContextId for an existing tab?
> 
restore into existing tabs could be an issue, but should be different from this bug, as mentioned above, so I'd like we move this discussion in bug 1257453.

> FWIW, the change to SessionStore.jsm seems fine but I don't understand the
> original issue, 
The change in SessionStore.jsm is that when duplicateTab is called, we didn't pass userContextId there, so the docshell created will not inherit the userContextId attribute from the tab.

This change is needed because 
https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/test/browser_sessionStoreContainer.js#12
wil need this change.

Originally it depends on SessionHistory to set origin attributes on the docshell, which is too late.


> SessionHistory.restore() is called before we kick off any
> document loads.
> 
But my assertion on the docshell (Part 2 patch) do trigger in most cases, for example, they use promiseTabStored(tab); which in turn will call docShell.setOriginAttributes, then trigger the assertion.

Thanks
Attachment #8729425 - Attachment description: WIP Part 1: don't set userContextId in SessionRestore → Part 1: don't set userContextId in SessionRestore
Attachment #8729425 - Flags: review?(ttaubert)
Depends on Bug 1271792, as I found my assertion will be triggered with the same back trace with it.

#0  0x00007f074ee9bba3 in nsDocShell::SetOriginAttributes(mozilla::DocShellOriginAttributes const&) (this=0x7f072e04e800, aAttrs=...) at /home/allstars/ssd/gecko-dev/docshell/base/nsDocShell.cpp:14093
#1  0x00007f074df760bd in mozilla::dom::TabChild::NotifyTabContextUpdated() (this=0x7f0732fa5000) at /home/allstars/ssd/gecko-dev/dom/ipc/TabChild.cpp:878
#2  0x00007f074df7bbf5 in mozilla::dom::TabChild::RecvSwappedWithOtherRemoteLoader(mozilla::dom::IPCTabContext const&) (this=0x7f0732fa5000, aContext=...)
    at /home/allstars/ssd/gecko-dev/dom/ipc/TabChild.cpp:2324
#3  0x00007f074b5ffbfd in mozilla::dom::PBrowserChild::OnMessageReceived(IPC::Message const&) (this=0x7f0732fa5060, msg__=...)
        at /home/allstars/ssd/gecko-dev/obj-x86_64-pc-linux-gnu/ipc/ipdl/PBrowserChild.cpp:3774
#4  0x00007f074b6ccbcb in mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) (this=0x7f073e389030, msg__=...)
            at /home/allstars/ssd/gecko-dev/obj-x86_64-pc-linux-gnu/ipc/ipdl/PContentChild.cpp:6351
#5  0x00007f074b133097 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) (this=0x7f073e389098, aMsg=...) at /home/allstars/ssd/gecko-dev/ipc/glue/MessageChannel.cpp:1655
#6  0x00007f074b132b45 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message const&) (this=0x7f073e389098, aMsg=...) at /home/allstars/ssd/gecko-dev/ipc/glue/MessageChannel.cpp:1593
#7  0x00007f074b132870 in mozilla::ipc::MessageChannel::OnMaybeDequeueOne() (this=0x7f073e389098) at /home/allstars/ssd/gecko-dev/ipc/glue/MessageChannel.cpp:1560
#8  0x00007f074b1516b4 in nsRunnableMethodArguments<>::applyImpl<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)()>(mozilla::ipc::MessageChannel*, bool (mozilla::ipc::MessageChannel::*)(), mozilla::Tuple<>&, mozilla::IndexSequence<>) (o=0x7f073e389098, m=(bool (mozilla::ipc::MessageChannel::*)(mozilla::ipc::MessageChannel * const)) 0x7f074b13273e <mozilla::ipc::MessageChannel::OnMaybeDequeueOne()>, args=...) at /home/allstars/ssd/gecko-dev/obj-x86_64-pc-linux-gnu/dist/include/nsThreadUtils.h:707
#9  0x00007f074b15132b in nsRunnableMethodArguments<>::apply<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)()>(mozilla::ipc::MessageChannel*, bool (mozilla::ipc::MessageChannel::*)()) (this=0x7f073e3a2378, o=0x7f073e389098, m=(bool (mozilla::ipc::MessageChannel::*)(mozilla::ipc::MessageChannel * const)) 0x7f074b13273e <mozilla::ipc::MessageChannel::OnMaybeDequeueOne()>)
                at /home/allstars/ssd/gecko-dev/obj-x86_64-pc-linux-gnu/dist/include/nsThreadUtils.h:714
#10 0x00007f074b150cc3 in nsRunnableMethodImpl<bool (mozilla::ipc::MessageChannel::*)(), false, true>::Run() (this=0x7f073e3a2340)
                    at /home/allstars/ssd/gecko-dev/obj-x86_64-pc-linux-gnu/dist/include/nsThreadUtils.h:741
#11 0x00007f074b13dda1 in mozilla::ipc::MessageChannel::RefCountedTask::Run() (this=0x7f073e3178b0) at /home/allstars/ssd/gecko-dev/obj-x86_64-pc-linux-gnu/dist/include/mozilla/ipc/MessageChannel.h:477
#12 0x00007f074b13df8e in mozilla::ipc::MessageChannel::DequeueTask::Run() (this=0x7f072c8c1b00) at /home/allstars/ssd/gecko-dev/obj-x86_64-pc-linux-gnu/dist/include/mozilla/ipc/MessageChannel.h:495
#13 0x00007f074b09ae1b in MessageLoop::RunTask(already_AddRefed<mozilla::Runnable>) (this=0x7ffe94dbe2c0, aTask=...) at /home/allstars/ssd/gecko-dev/ipc/chromium/src/base/message_loop.cc:335
#14 0x00007f074b09aec6 in MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask&&) (this=0x7ffe94dbe2c0, pending_task=<unknown type in /home/allstars/ssd/gecko-dev/obj-x86_64-pc-linux-gnu/dist/bin/libxul.so, CU 0x1e709db, DIE 0x1eb9237>) at /home/allstars/ssd/gecko-dev/ipc/chromium/src/base/message_loop.cc:343
#15 0x00007f074b09b2c0 in MessageLoop::DoWork() (this=0x7ffe94dbe2c0) at /home/allstars/ssd/gecko-dev/ipc/chromium/src/base/message_loop.cc:418
#16 0x00007f074b137716 in mozilla::ipc::DoWorkRunnable::Run() (this=0x7f073e372790) at /home/allstars/ssd/gecko-dev/ipc/glue/MessagePump.cpp:227
#17 0x00007f074a926fda in nsThread::ProcessNextEvent(bool, bool*) (this=0x7f073bdef500, aMayWait=true, aResult=0x7ffe94dbdef7) at /home/allstars/ssd/gecko-dev/xpcom/threads/nsThread.cpp:991
#18 0x00007f074a995e95 in NS_ProcessNextEvent(nsIThread*, bool) (aThread=0x7f073bdef500, aMayWait=true) at /home/allstars/ssd/gecko-dev/xpcom/glue/nsThreadUtils.cpp:290
#19 0x00007f074b13717b in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (this=0x7f073e377380, aDelegate=0x7ffe94dbe2c0) at /home/allstars/ssd/gecko-dev/ipc/glue/MessagePump.cpp:130
#20 0x00007f074b137a54 in mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (this=0x7f073e377380, aDelegate=0x7ffe94dbe2c0)
                        at /home/allstars/ssd/gecko-dev/ipc/glue/MessagePump.cpp:303
#21 0x00007f074b09a74a in MessageLoop::RunInternal() (this=0x7ffe94dbe2c0) at /home/allstars/ssd/gecko-dev/ipc/chromium/src/base/message_loop.cc:233
#22 0x00007f074b09a6ce in MessageLoop::RunHandler() (this=0x7ffe94dbe2c0) at /home/allstars/ssd/gecko-dev/ipc/chromium/src/base/message_loop.cc:226
#23 0x00007f074b09a692 in MessageLoop::Run() (this=0x7ffe94dbe2c0) at /home/allstars/ssd/gecko-dev/ipc/chromium/src/base/message_loop.cc:206
#24 0x00007f074e317798 in nsBaseAppShell::Run() (this=0x7f0738fa4320) at /home/allstars/ssd/gecko-dev/widget/nsBaseAppShell.cpp:156
#25 0x00007f074f40f27f in XRE_RunAppShell() () at /home/allstars/ssd/gecko-dev/toolkit/xre/nsEmbedFunctions.cpp:802
#26 0x00007f074b1378e9 in mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (this=0x7f073e377380, aDelegate=0x7ffe94dbe2c0)
                            at /home/allstars/ssd/gecko-dev/ipc/glue/MessagePump.cpp:271
#27 0x00007f074b09a74a in MessageLoop::RunInternal() (this=0x7ffe94dbe2c0) at /home/allstars/ssd/gecko-dev/ipc/chromium/src/base/message_loop.cc:233
#28 0x00007f074b09a6ce in MessageLoop::RunHandler() (this=0x7ffe94dbe2c0) at /home/allstars/ssd/gecko-dev/ipc/chromium/src/base/message_loop.cc:226
#29 0x00007f074b09a692 in MessageLoop::Run() (this=0x7ffe94dbe2c0) at /home/allstars/ssd/gecko-dev/ipc/chromium/src/base/message_loop.cc:206
#30 0x00007f074f40ec75 in XRE_InitChildProcess(int, char**, mozilla::gmp::GMPLoader*) (aArgc=3, aArgv=0x7ffe94dbf7f8, aGMPLoader=0x0) at /home/allstars/ssd/gecko-dev/toolkit/xre/nsEmbedFunctions.cpp:637
#31 0x0000000000427cfd in content_process_main(int, char**) (argc=5, argv=0x7ffe94dbf7f8) at /home/allstars/ssd/gecko-dev/ipc/app/../contentproc/plugin-container.cpp:237
#32 0x0000000000427dda in main(int, char**) (argc=6, argv=0x7ffe94dbf7f8) at /home/allstars/ssd/gecko-dev/ipc/app/MozillaRuntimeMain.cpp:11
Depends on: 1271792
(In reply to Yoshi Huang[:allstars.chh] from comment #44)
> Depends on Bug 1271792, as I found my assertion will be triggered with the
> same back trace with it.
It has similar traces but I think they are not entirely the same problem, removing dependency for now.
No longer depends on: 1271792
Hi, Jonas
See the trace from Comment 44, now when swapFrameLoaders is called, in the end it will update the origin attributes on the docshell (top-level docshell in TabChild), and it will trigger the assertion as mContentViewer also loads something, can you suggest what we should do here?

Thanks
Flags: needinfo?(jonas)
Comment on attachment 8729425 [details] [diff] [review]
Part 1: don't set userContextId in SessionRestore

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

Please adjust the patch title. It does only implement and test correct userContextId handling for duplicateTab().

::: browser/components/sessionstore/SessionHistory.jsm
@@ -266,5 @@
> -    if ("userContextId" in tabData) {
> -      let attrs = docShell.getOriginAttributes();
> -      attrs.userContextId = tabData.userContextId;
> -      docShell.setOriginAttributes(attrs);
> -    }

If we remove that, shouldn't we also remove the setUserContextId() call from restoreTab()? That will have to be properly implement with bug 1257453, right? The userContextId collection can stay, we'll need that for the aforementioned bug.
Attachment #8729425 - Flags: review?(ttaubert) → review+
(In reply to Tim Taubert [:ttaubert] from comment #47)
> ::: browser/components/sessionstore/SessionHistory.jsm
> @@ -266,5 @@
> > -    if ("userContextId" in tabData) {
> > -      let attrs = docShell.getOriginAttributes();
> > -      attrs.userContextId = tabData.userContextId;
> > -      docShell.setOriginAttributes(attrs);
> > -    }
> 
> If we remove that, shouldn't we also remove the setUserContextId() call from
> restoreTab()? That will have to be properly implement with bug 1257453,
> right? The userContextId collection can stay, we'll need that for the
> aforementioned bug.
Agree, I'll add a TODO there.

Thanks for the review.
Since the origin attributes will be the same at the time this function is called, so I remove the call of notifyTabContextUpdated(), and just set the frameType on the docshell. This should prevents the crash I mentioned earlier.
Flags: needinfo?(jonas)
Summary: mContentViewer should be null when setting origin attributes on the docshell → mContentViewer shouldn't load any document except for about:blank when setting origin attributes on the docshell
Comment on attachment 8752679 [details] [diff] [review]
Part 2: mContentViewer shouldn't load any document except for about:blank when setting origin attributes on docshell.

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

Hi Jonas
So far there are some places will trigger the assertion for mContentViewer is null.
But this bug has been a while so I'd like to start with checking it if is about:blank first,
I've filed a seperate bug 1273058.

Could you review this for me ?

Thanks
Attachment #8752679 - Flags: review?(jonas)
Comment on attachment 8752660 [details] [diff] [review]
Part 3: don't call notifyTabContextUpdated() when swapFrameLoader

Hi Jonas

For this patch, I found when swapFrameLoader is called, it will check if two frame loaders have the same origin attributes, if it doesn't it will bail out in parent side.

So at the time TabChild::RecvSwappedWithOtherRemoteLoader is called, the docshell should have the same origin attributes with the docshell in the other fame loader. Therefore calling NotifyTabContextUpdated should *not* be neccesary. 

PS: Originally I plan to send r? to bz since he reviewed Bug 1242644, but he doesn't accept r? now.

Thanks
Attachment #8752660 - Attachment description: WIP - Part 3: don't call notifyTabContextUpdated() → Part 3: don't call notifyTabContextUpdated() when swapFrameLoader
Attachment #8752660 - Flags: review?(jonas)
(In reply to Yoshi Huang[:allstars.chh] from comment #48)
> (In reply to Tim Taubert [:ttaubert] from comment #47)
> > ::: browser/components/sessionstore/SessionHistory.jsm
> > @@ -266,5 @@
> > > -    if ("userContextId" in tabData) {
> > > -      let attrs = docShell.getOriginAttributes();
> > > -      attrs.userContextId = tabData.userContextId;
> > > -      docShell.setOriginAttributes(attrs);
> > > -    }
> > 
> > If we remove that, shouldn't we also remove the setUserContextId() call from
> > restoreTab()? That will have to be properly implement with bug 1257453,
> > right? The userContextId collection can stay, we'll need that for the
> > aforementioned bug.
> Agree, I'll add a TODO there.
> 
> Thanks for the review.
In fact this line could be removed entirely since duplicateTab already passes userContextId to the new tab. So if try doesn't have any failure I'll remove it.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0429af0097b
Comment on attachment 8752660 [details] [diff] [review]
Part 3: don't call notifyTabContextUpdated() when swapFrameLoader

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

r=me with the below fixed.

::: dom/ipc/TabChild.cpp
@@ +2384,5 @@
> +  docShell->SetFrameType(IsMozBrowserElement() ?
> +                           nsIDocShell::FRAME_TYPE_BROWSER :
> +                           HasOwnApp() ?
> +                             nsIDocShell::FRAME_TYPE_APP :
> +                             nsIDocShell::FRAME_TYPE_REGULAR);

I take it this is needed because UpdateTabContextAfterSwap still allows mIsMozBrowserElement to be changed?

Could you break this out into a separate function, which is called both here and from NotifyTabContextUpdated?

And a minor nit: I prefer expressions like this to be indented more like if-elseif-else statements:

IsMozBrowserElement() ? nsIDocShell::FRAME_TYPE_BROWSER :
HasOwnApp() ? nsIDocShell::FRAME_TYPE_APP :
nsIDocShell::FRAME_TYPE_REGULAR
Attachment #8752660 - Flags: review?(jonas) → review+
Comment on attachment 8752679 [details] [diff] [review]
Part 2: mContentViewer shouldn't load any document except for about:blank when setting origin attributes on docshell.

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

::: docshell/base/nsDocShell.cpp
@@ +14084,5 @@
> +  if (mContentViewer) {
> +    nsIDocument* doc = mContentViewer->GetDocument();
> +    if (doc) {
> +      nsIURI* uri = doc->GetDocumentURI();
> +      if (uri) {

I'd say you should MOZ_ASSERT(uri) as well. I don't know of any instance when uri is null, but if it is, it's at least worth making sure we don't attempt to change the OriginAttributes of such a document.
Attachment #8752679 - Flags: review?(jonas) → review+
(In reply to Jonas Sicking (:sicking) from comment #54)
> ::: dom/ipc/TabChild.cpp
> @@ +2384,5 @@
> > +  docShell->SetFrameType(IsMozBrowserElement() ?
> > +                           nsIDocShell::FRAME_TYPE_BROWSER :
> > +                           HasOwnApp() ?
> > +                             nsIDocShell::FRAME_TYPE_APP :
> > +                             nsIDocShell::FRAME_TYPE_REGULAR);
> 
> I take it this is needed because UpdateTabContextAfterSwap still allows
> mIsMozBrowserElement to be changed?
> 
Yes. I'll add comments there so make it more clear.

Thanks for the review.
updated patch subject and removed setUserContextId from SessionStore.jsm
Attachment #8729425 - Attachment is obsolete: true
Attachment #8753327 - Flags: review+
moved docShell->SetFrameType to a seperate function
indent
And move this patch to Part 2.
Attachment #8752660 - Attachment is obsolete: true
Attachment #8753329 - Flags: review+
Awesome! Yoshi, thanks for fixing this bug!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: