1318388 - "Private Tab" add-on cause crash in mozilla::dom::PostMessageEvent::Run

Status: VERIFIED FIXED

(Firefox :: Extension Compatibility, defect)

Description

[Infocatcher]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20161019084923

Steps to reproduce:

See
https://bugzilla.mozilla.org/show_bug.cgi?id=1315233#c24
https://bugzilla.mozilla.org/show_bug.cgi?id=1315233#c27


Actual results:

Firefox crashes.
Anyway, I'm unable to reproduce it myself...


Expected results:

Firefox shouldn't crashes.

Comment 1

[Infocatcher]

Also see
https://bugzilla.mozilla.org/show_bug.cgi?id=1315233#c29
and next one: https://bugzilla.mozilla.org/show_bug.cgi?id=1315233#c30

Comment 2

[Kris Maglione [:kmag]]

Baku, Ehsan, do either of you happen to know if there's a reasonably safe way for this add-on to mark a single tab in a non-private window private?

As far as I know, this is still intentionally unsupported. But I haven't been following the user context work closely, and it seems like that might have made this more tenable than it used to be.

(In reply to Infocatcher from comment #0)
> Firefox crashes.
> Anyway, I'm unable to reproduce it myself...

You'll only see these crashes in Developer Edition and nightly, and currently only when a non-private window attempts to post a message to a private window, or vice versa. I'm not sure under exactly which circumstances your extension causes this, but I am sure that it does cause it.

Comment 3

[(no longer active)]

A private tab in a non-private window isn't a configuration that we support.  However I don't think we should crash in this case.  Can you please provide a crash stats report link so that I can take a look at the crash and think of a way to bypass it?  Also, is the source to this add-on available somewhere in the web?

Thanks!

Comment 4

[Kris Maglione [:kmag]]

The crash is caused by a diagnostic assert, added in bug 1308920, that asserts that window.postMessage is never called across origins with the same origin string but different origin attributes.

Comment 5

[Andrea Marchesini [:baku]]

> A private tab in a non-private window isn't a configuration that we support.
> However I don't think we should crash in this case.  Can you please provide

I don't see why we should support this case. A crash seems reasonable to me.
Why should we support it?

Comment 6

[Kris Maglione [:kmag]]

I agree that crashing is probably the right thing when we run into this situation.

My main question is, is there any safe way to set the private browsing ID of a single tab without risking giving that tab access to windows with different origin attributes?

Comment 7

[(no longer active)]

(In reply to Andrea Marchesini [:baku] from comment #5)
> > A private tab in a non-private window isn't a configuration that we support.
> > However I don't think we should crash in this case.  Can you please provide
> 
> I don't see why we should support this case. A crash seems reasonable to me.
> Why should we support it?

Firstly, looking at <https://hg.mozilla.org/mozilla-central/rev/36b37ea010f1>, the logic there is just incorrect as far as I can tell.  The if condition checks the principals ignoring the add-on ID (add-on ID being a member of OA!) and then the assertion expects the OAs to always be the same?!  At the very least that should use ChromeUtils::IsOriginAttributesEqualIgnoringAddonId() instead.

But more importantly, did you note that we were about to print a console diagnostic message, before asserting?  We are turning what would be a console message that the add-on developer could understanding into a crash that they won't know what to do with.  I'm fine with not supporting this case, I am however arguing that we should not crash.

Comment 8

[(no longer active)]

(In reply to Kris Maglione [:kmag] from comment #6)
> I agree that crashing is probably the right thing when we run into this
> situation.

I disagree!  We *are* printing a console message and bailing out here, no point to crash.

> My main question is, is there any safe way to set the private browsing ID of
> a single tab without risking giving that tab access to windows with
> different origin attributes?

Generally no, since you can never know whether some code has previously looked at your OA and made decisions based on the privateBrowsingId member, so in general that would leave things into a potentially unstable state.

That being said, I personally see nothing wrong with an add-on doing this, as long as it was made clear in the add-on description that the add-on could break the user's privacy in some edge cases.  I hope this is something that AMO reviewers are flagging...

Comment 9

[(no longer active)]

(In reply to :Ehsan Akhgari from comment #7)
> (In reply to Andrea Marchesini [:baku] from comment #5)
> > > A private tab in a non-private window isn't a configuration that we support.
> > > However I don't think we should crash in this case.  Can you please provide
> > 
> > I don't see why we should support this case. A crash seems reasonable to me.
> > Why should we support it?
> 
> Firstly, looking at
> <https://hg.mozilla.org/mozilla-central/rev/36b37ea010f1>, the logic there
> is just incorrect as far as I can tell.  The if condition checks the
> principals ignoring the add-on ID (add-on ID being a member of OA!) and then
> the assertion expects the OAs to always be the same?!  At the very least
> that should use ChromeUtils::IsOriginAttributesEqualIgnoringAddonId()
> instead.

Bug 1314361 is removing add-on ID from OA, so this part of my comment is now less important.

Comment 10

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari from comment #7)
> Firstly, looking at
> <https://hg.mozilla.org/mozilla-central/rev/36b37ea010f1>, the logic there
> is just incorrect as far as I can tell.  The if condition checks the
> principals ignoring the add-on ID (add-on ID being a member of OA!) and then
> the assertion expects the OAs to always be the same?!  At the very least
> that should use ChromeUtils::IsOriginAttributesEqualIgnoringAddonId()
> instead.

It only expects the OAs to be the same if the origin strings are the same. The
only time that should ever come up is for the null principal. If we ever get
to that point and the origin strings are the same and the attributes differ
only by add-on ID, something is still very wrong.

The particular issue I was trying to deal with was that when we wound up in
that condition, we printed a console message saying that the postMessage
failed because the origins didn't match, but the two origin strings were the
same.

> But more importantly, did you note that we were about to print a console
> diagnostic message, before asserting?  We are turning what would be a
> console message that the add-on developer could understanding into a crash
> that they won't know what to do with.  I'm fine with not supporting this
> case, I am however arguing that we should not crash.

The console message wouldn't be something the add-on developer could
understand. That was the problem. My initial plan was to add origin attribute
suffixes to both origins when the origin strings matched, but after going over
it with Boris, we agreed that we shouldn't actually ever get into that state.

The diagnostic assert has already caught bugs that we wouldn't have known
about otherwise, one in the built-in SDK code, and one (or several) in this
add-on. Given that we never expect internal code to put us in this situation,
and we shouldn't expect add-ons trying to implement an unsupported
configuration to put us in it either, I still think that crashing is the most
appropriate response to it.

Comment 11

[(no longer active)]

(In reply to Kris Maglione [:kmag] from comment #10)
> (In reply to :Ehsan Akhgari from comment #7)
> > Firstly, looking at
> > <https://hg.mozilla.org/mozilla-central/rev/36b37ea010f1>, the logic there
> > is just incorrect as far as I can tell.  The if condition checks the
> > principals ignoring the add-on ID (add-on ID being a member of OA!) and then
> > the assertion expects the OAs to always be the same?!  At the very least
> > that should use ChromeUtils::IsOriginAttributesEqualIgnoringAddonId()
> > instead.
> 
> It only expects the OAs to be the same if the origin strings are the same.
> The
> only time that should ever come up is for the null principal. If we ever get
> to that point and the origin strings are the same and the attributes differ
> only by add-on ID, something is still very wrong.
> 
> The particular issue I was trying to deal with was that when we wound up in
> that condition, we printed a console message saying that the postMessage
> failed because the origins didn't match, but the two origin strings were the
> same.

Oh, OK now I understand what's going on.  In this case I would like to know more about why the assertion fails with this exception.  Where are these principals coming from?  Do you know?

> > But more importantly, did you note that we were about to print a console
> > diagnostic message, before asserting?  We are turning what would be a
> > console message that the add-on developer could understanding into a crash
> > that they won't know what to do with.  I'm fine with not supporting this
> > case, I am however arguing that we should not crash.
> 
> The console message wouldn't be something the add-on developer could
> understand. That was the problem. My initial plan was to add origin attribute
> suffixes to both origins when the origin strings matched, but after going
> over
> it with Boris, we agreed that we shouldn't actually ever get into that state.

Yes, I agree with that now that I understand what's involved more.  :-)

> The diagnostic assert has already caught bugs that we wouldn't have known
> about otherwise, one in the built-in SDK code, and one (or several) in this
> add-on. Given that we never expect internal code to put us in this situation,
> and we shouldn't expect add-ons trying to implement an unsupported
> configuration to put us in it either, I still think that crashing is the most
> appropriate response to it.

At least to me it's still not clear that this isn't a real bug though.  It really depends on where these principals are coming from.

Comment 12

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari from comment #11)
> (In reply to Kris Maglione [:kmag] from comment #10)
> > The particular issue I was trying to deal with was that when we wound up in
> > that condition, we printed a console message saying that the postMessage
> > failed because the origins didn't match, but the two origin strings were the
> > same.
>
> Oh, OK now I understand what's going on.  In this case I would like to know
> more about why the assertion fails with this exception.  Where are these
> principals coming from?  Do you know?

The expected target principal is constructed from the origin string passed to
postMessage and the origin attributes of the caller. The other principal is
the actual principal of the page.

In the particular case of the crash, I'm not sure what the source and target
are. It could be two frames in the same content tree, or a tab opened by
window.open. But it almost doesn't matter. From what I can tell, the add-on
currently toggles the usePrivateBrowsing attribute of docshells and of channel
load groups at various times, which is definitely not safe. So whatever it's
doing now, it's going to have to start doing something almost completely
different in the future.

Comment 13

[(no longer active)]

(In reply to Kris Maglione [:kmag] from comment #12)
> (In reply to :Ehsan Akhgari from comment #11)
> > (In reply to Kris Maglione [:kmag] from comment #10)
> > > The particular issue I was trying to deal with was that when we wound up in
> > > that condition, we printed a console message saying that the postMessage
> > > failed because the origins didn't match, but the two origin strings were the
> > > same.
> >
> > Oh, OK now I understand what's going on.  In this case I would like to know
> > more about why the assertion fails with this exception.  Where are these
> > principals coming from?  Do you know?
> 
> The expected target principal is constructed from the origin string passed to
> postMessage and the origin attributes of the caller. The other principal is
> the actual principal of the page.
> 
> In the particular case of the crash, I'm not sure what the source and target
> are. It could be two frames in the same content tree, or a tab opened by
> window.open.

Well, in the case of the former scenario we probably have a serious bug that we need to fix, no?

If however this is the add-on code calling postMessage manually in some weird way with the wrong pbId, then I agree that the assertion failure itself doesn't matter.  Perhaps in that case we can do something better than crashing?

Do you have a backtrace to the crash?

> But it almost doesn't matter. From what I can tell, the add-on
> currently toggles the usePrivateBrowsing attribute of docshells and of
> channel
> load groups at various times, which is definitely not safe. So whatever it's
> doing now, it's going to have to start doing something almost completely
> different in the future.

I'm not sure what you're suggesting the add-on should do instead.  I can't think of another way to do this...

Comment 14

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari from comment #13)
> Well, in the case of the former scenario we probably have a serious bug that
> we need to fix, no?

I'm not sure what you mean.

> If however this is the add-on code calling postMessage manually in some
> weird way with the wrong pbId, then I agree that the assertion failure
> itself doesn't matter.  Perhaps in that case we can do something better than
> crashing?

The problem isn't that the add-on is calling postMessage, it's that it's
allowing a private browsing content window to get a reference to a non-private
window, or vice versa, and attempt to post a message to it.

> Do you have a backtrace to the crash?

There are backtraces in crashstats, but they're not useful, since this always
happens from a runnable dispatched from the main thread event loop.

> I'm not sure what you're suggesting the add-on should do instead.  I can't
> think of another way to do this...

I'm not sure what it should do instead, but I am sure that it shouldn't ever
change the private browsing ID of a docShell except immediately after it's
been created, and it *definitely* shouldn't change it based on a channel being
loaded into it or one of its descendents.

Comment 15

[Andrea Marchesini [:baku]]

It seems to me that we should implement a custom check for this scenario.
How complex is to write a test case? I would like to debug it locally.

Comment 16

[(no longer active)]

(In reply to Kris Maglione [:kmag] from comment #14)
> (In reply to :Ehsan Akhgari from comment #13)
> > Well, in the case of the former scenario we probably have a serious bug that
> > we need to fix, no?
> 
> I'm not sure what you mean.

If the reason that OAs are different is that for example the add-on is messing with the docshell flag on a docshell that belongs to an iframe on a web page then the crash is a direct result of the add-on doing something bad.  But if we get confused somehow and end up handing a reference to a private window to a non-private window or something like that, then there is a great chance that we may have a bug in our code which we may need to fix.

> > If however this is the add-on code calling postMessage manually in some
> > weird way with the wrong pbId, then I agree that the assertion failure
> > itself doesn't matter.  Perhaps in that case we can do something better than
> > crashing?
> 
> The problem isn't that the add-on is calling postMessage, it's that it's
> allowing a private browsing content window to get a reference to a
> non-private
> window, or vice versa, and attempt to post a message to it.

Do you know how this reference is obtained?  Unless if it's something the add-on is explicitly doing, then that's a serious bug.

> > Do you have a backtrace to the crash?
> 
> There are backtraces in crashstats, but they're not useful, since this always
> happens from a runnable dispatched from the main thread event loop.

Yeah, I looked at a few of them, you're right.

The next steis for us to debug this locally.  Andrea, can you please take that on?  I'd like to know the answer to three questions:

1. What parts of the OAs are different?
2. How is the add-on getting a reference to the window with a different OA.
3. Where the two principals are coming from?

> > I'm not sure what you're suggesting the add-on should do instead.  I can't
> > think of another way to do this...
> 
> I'm not sure what it should do instead, but I am sure that it shouldn't ever
> change the private browsing ID of a docShell except immediately after it's
> been created, and it *definitely* shouldn't change it based on a channel
> being
> loaded into it or one of its descendents.

My question wasn't rhetorical.  Currently the *only* way that an extension can make a private tab that I'm aware of is by calling the setter of nsILoadContext.usePrivateBrowsing.  It looks like these days the only scripted callers of this setter are tests in our tree, so we can probably stop exposing this setter through script and avoid such bad usages, but then essentially we'd be killing this extension.  We could perhaps add a tabbrowser-level API (or a WebExtension API) for making a tab private and maybe enforce things like it only working on a tab or when the extension gets to use the API...  Is there any APIs related to private browsing in the WebExtensions world right now?

Comment 17

[(no longer active)]

(In reply to Andrea Marchesini [:baku] from comment #15)
> It seems to me that we should implement a custom check for this scenario.
> How complex is to write a test case? I would like to debug it locally.

I really think we need to debug the extension itself locally.  The last time we found a misbehaving extensions in PB, that turned out to be a huge bug on our side.  I'd hate to quickly move to debugging a reduced test case before we know *exactly* what leads to the crash with the add-on...

Comment 18

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari from comment #16)
> (In reply to Kris Maglione [:kmag] from comment #14)
> > The problem isn't that the add-on is calling postMessage, it's that it's
> > allowing a private browsing content window to get a reference to a
> > non-private
> > window, or vice versa, and attempt to post a message to it.
>
> Do you know how this reference is obtained?  Unless if it's something the
> add-on is explicitly doing, then that's a serious bug.

No. My suspicion is that either the extension is either changing the OAs of
some frame in the docshell tree after it's created, but I haven't tried to
reproduce this yet. I'll try today.

> > I'm not sure what it should do instead, but I am sure that it shouldn't ever
> > change the private browsing ID of a docShell except immediately after it's
> > been created, and it *definitely* shouldn't change it based on a channel
> > being loaded into it or one of its descendents.
>
> My question wasn't rhetorical.

Sorry, my answer wasn't rhetorical either.

> Currently the *only* way that an extension can make a private tab that I'm
> aware of is by calling the setter of nsILoadContext.usePrivateBrowsing.

It should also be possible by calling docShell.setOriginAttributes()

But the specific problem I have with the add-on is not so much that it sets
usePrivateBrowsing so much as that it sets it to different values throughout
the docShell's lifetime, rather than just once, immediately after it's
created. For instance, it registers a private: protocol handler, and if any
URL from that handler is loaded into a docShell, it toggles its
usePrivateBrowsing flag to true.

> It looks like these days the only scripted callers of this setter are tests
> in our tree, so we can probably stop exposing this setter through script and
> avoid such bad usages, but then essentially we'd be killing this extension.
> We could perhaps add a tabbrowser-level API (or a WebExtension API) for
> making a tab private and maybe enforce things like it only working on a tab
> or when the extension gets to use the API... Is there any APIs related to
> private browsing in the WebExtensions world right now?

Yes, tabs and windows have an 'incognito' property. It's currently possible to
explicitly open private or non-private windows, but not private or non-private
tabs. Currently, we actively prevent private tabs from being moved to
non-private windows, and vice versa.

I'd be wary of adding that functionality unless we plan to properly support
it, though. And add some sort of UI element to indicate that a tab is private.

Comment 19

[(no longer active)]

(In reply to Kris Maglione [:kmag] from comment #18)
> (In reply to :Ehsan Akhgari from comment #16)
> > (In reply to Kris Maglione [:kmag] from comment #14)
> > > The problem isn't that the add-on is calling postMessage, it's that it's
> > > allowing a private browsing content window to get a reference to a
> > > non-private
> > > window, or vice versa, and attempt to post a message to it.
> >
> > Do you know how this reference is obtained?  Unless if it's something the
> > add-on is explicitly doing, then that's a serious bug.
> 
> No. My suspicion is that either the extension is either changing the OAs of
> some frame in the docshell tree after it's created, but I haven't tried to
> reproduce this yet. I'll try today.

Thank you!  I hope your suspicion is correct, FWIW.  :-)

> > > I'm not sure what it should do instead, but I am sure that it shouldn't ever
> > > change the private browsing ID of a docShell except immediately after it's
> > > been created, and it *definitely* shouldn't change it based on a channel
> > > being loaded into it or one of its descendents.
> >
> > My question wasn't rhetorical.
> 
> Sorry, my answer wasn't rhetorical either.
> 
> > Currently the *only* way that an extension can make a private tab that I'm
> > aware of is by calling the setter of nsILoadContext.usePrivateBrowsing.
> 
> It should also be possible by calling docShell.setOriginAttributes()

Ah of course!

(We may wanna close that hole...  I definitely didn't intend to open up another scriptable API for this...  At least the other one warns <http://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#2206>)

> But the specific problem I have with the add-on is not so much that it sets
> usePrivateBrowsing so much as that it sets it to different values throughout
> the docShell's lifetime, rather than just once, immediately after it's
> created. For instance, it registers a private: protocol handler, and if any
> URL from that handler is loaded into a docShell, it toggles its
> usePrivateBrowsing flag to true.

Oh, that's definitely not OK...  Depending on exactly what the add-on is doing we can throw exceptions from SetUsePrivateBrowsing() for example if InternalLoad has already been called.

I was hoping the add-on does something more benign like just calling setUsePrivateBrowsing on the docshell corresponding to a <xul:browser> for a tab...

> > It looks like these days the only scripted callers of this setter are tests
> > in our tree, so we can probably stop exposing this setter through script and
> > avoid such bad usages, but then essentially we'd be killing this extension.
> > We could perhaps add a tabbrowser-level API (or a WebExtension API) for
> > making a tab private and maybe enforce things like it only working on a tab
> > or when the extension gets to use the API... Is there any APIs related to
> > private browsing in the WebExtensions world right now?
> 
> Yes, tabs and windows have an 'incognito' property. It's currently possible
> to
> explicitly open private or non-private windows, but not private or
> non-private
> tabs. Currently, we actively prevent private tabs from being moved to
> non-private windows, and vice versa.

Yeah, we do the same in the UI as well (to prevent people from moving a private tab into a non-private window and vice versa.)

> I'd be wary of adding that functionality unless we plan to properly support
> it, though. And add some sort of UI element to indicate that a tab is
> private.

We can never support this properly in the sense that makes _me_ happy due to the issue in comment 8.  Do we know if there are other extensions that try to do something similar to this?  While I could be convinced of supporting some basic way for an add-on to set a newly opened tab in PB mode, I have no desire to support the kind of thing you described above.

Comment 20

[Kris Maglione [:kmag]]

I didn't have much time to look into this earlier in the week, but I've come up with at least one way to reproduce this now:

- Create a document (a.html) that uses window.open to create a new tab containing a document (b.html) which calls `opener.sendMessage()` with an origin string that matches the first document.
- Load the a.html in a private tab. It will open a new private tab containing b.html.
- Toggle the tab containing a.html to a non-private tab.
- Have b.html post a message to it.
- Crash.

I suspect there are other ways to trigger this, and other corner cases where it comes up, but that's the first one I managed to trigger.

(In reply to :Ehsan Akhgari from comment #19)
> (We may wanna close that hole...  I definitely didn't intend to open up
> another scriptable API for this...  At least the other one warns
> <http://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.
> cpp#2206>)

Well, it at least has the advantage of only working before any document has
been loaded into the docShell. usePrivateBrowsing can be set at any time.

If we do disable this, though, we're going to need to come up with another
workaround for SDK panels being shown in private windows, and add support for
directly creating private windowless browsers for WebExtension background
pages.

> Oh, that's definitely not OK...  Depending on exactly what the add-on is
> doing we can throw exceptions from SetUsePrivateBrowsing() for example if
> InternalLoad has already been called.

Yeah, that's probably a good idea.

> > I'd be wary of adding that functionality unless we plan to properly support
> > it, though. And add some sort of UI element to indicate that a tab is
> > private.
>
> We can never support this properly in the sense that makes _me_ happy due to
> the issue in comment 8.

Well, I think I'd be happy if we at least support it to the extent that we
support user context IDs per-tab, and make it clear to users of the API that
there are no guarantees. It would at least be better than having them hack
around our current lack of support.

I agree that we shouldn't allow ever switching an already initialized docshell
into or out of private browsing, though.

> Do we know if there are other extensions that try to do something similar to
> this?  While I could be convinced of supporting some basic way for an add-on
> to set a newly opened tab in PB mode, I have no desire to support the kind
> of thing you described above.

Well, there's at least these:

https://addons.mozilla.org/addon/auto-private/
https://addons.mozilla.org/addon/switch-private-browsing/
https://addons.mozilla.org/addon/minibrowserx/

And according to DXR, a few others that set appear to set usePrivateBrowsing
on their own windows for reasons I don't entirely understand.

Comment 21

[(no longer active)]

(In reply to Kris Maglione [:kmag] from comment #20)
> I didn't have much time to look into this earlier in the week, but I've come
> up with at least one way to reproduce this now:
> 
> - Create a document (a.html) that uses window.open to create a new tab
> containing a document (b.html) which calls `opener.sendMessage()` with an
> origin string that matches the first document.
> - Load the a.html in a private tab. It will open a new private tab
> containing b.html.
> - Toggle the tab containing a.html to a non-private tab.
> - Have b.html post a message to it.
> - Crash.
> 
> I suspect there are other ways to trigger this, and other corner cases where
> it comes up, but that's the first one I managed to trigger.

Ah hmm, if this is the way to trigger the crash, then there's really not much that we can do besides closing this bug.  :/

Comment 22

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari from comment #21)
> Ah hmm, if this is the way to trigger the crash, then there's really not
> much that we can do besides closing this bug.  :/

Well, I think it's worth at least changing the usePrivateBrowsing setter to throw once anything has been loaded into the docshell.

Comment 23

[(no longer active)]

Yeah, that sounds good to me.

Comment 24

[Kris Maglione [:kmag]]

Attachment: Bug 1318388: Prevent setting usePrivateBrowsing after initial document load.

Review commit: https://reviewboard.mozilla.org/r/99368/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99368/

Comment 25

[Kris Maglione [:kmag]]

Comment on attachment 8819698 [details]
Bug 1318388: Prevent setting usePrivateBrowsing after initial document load.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99368/diff/1-2/

Comment 26

[(no longer active)]

Comment on attachment 8819698 [details]
Bug 1318388: Prevent setting usePrivateBrowsing after initial document load.

https://reviewboard.mozilla.org/r/99368/#review99892

::: docshell/test/unit/test_setUsePrivateBrowsing.js:32
(Diff revision 2)
> +  yield new Promise(do_execute_soon);
> +
> +  // This causes a failed assertion rather than an exception on debug
> +  // builds.
> +  if (!AppConstants.DEBUG) {
> +    Assert.throws(() => { loadContext.usePrivateBrowsing = true; },

Your patch also fixes the docShell.originAttributes setter.  Can you please extend the test to cover that too, and amend the commit message accordingly?  Thanks!

Comment 27

[Kris Maglione [:kmag]]

Comment on attachment 8819698 [details]
Bug 1318388: Prevent setting usePrivateBrowsing after initial document load.

https://reviewboard.mozilla.org/r/99368/#review99892

> Your patch also fixes the docShell.originAttributes setter.  Can you please extend the test to cover that too, and amend the commit message accordingly?  Thanks!

I just applied the existing logic from setOriginAttributes to both setOriginAttributes and usePrivateBrowsing. But the former doesn't appear to have an existing test, so I'll add a test for it here anyway.

Comment 28

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac719fd12bc9e2541adee844f3874a200d1f4749
Bug 1318388: Prevent setting usePrivateBrowsing after initial document load. r=ehsan

Comment 29

[Kris Maglione [:kmag]]

Comment on attachment 8819698 [details]
Bug 1318388: Prevent setting usePrivateBrowsing after initial document load.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1308920

[User impact if declined]: This bug causes a crash due to a failed assertion in non-release builds when add-ons change private browsing states in intentionally unsupported ways.

[Is this code covered by automated tests?]: Yes, this patch adds tests for this issue.

[Has the fix been verified in Nightly?]: No.
[Needs manual test from QE? If yes, steps to reproduce]: We should verify that the steps in comment 20 no longer produce a crash.

[List of other uplifts needed for the feature/fix]: None.

[Is the change risky?]: Somewhat, but relatively low-risk.
[Why is the change risky/not risky?]: This change will prevent add-ons like Private Tab from changing the private browsing state of existing tabs. However, the changes that it prevents are dangerous, and cause crashes in non-release builds, so the risk of not uplifting is greater.

[String changes made/needed]: None.

Comment 30

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8de88776f656609009d371e9b71ccea8c2fe7615
Bug 1318388: Follow-up: Fix test crash in debug builds.

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ac719fd12bc9
https://hg.mozilla.org/mozilla-central/rev/8de88776f656

Comment 32

[Julien Cristau [:jcristau]]

Comment 20 provides steps to reproduce a crash this is fixing, would be good to have verification in nightly (and aurora once this is uplifted).

Comment 33

[Julien Cristau [:jcristau]]

Comment on attachment 8819698 [details]
Bug 1318388: Prevent setting usePrivateBrowsing after initial document load.

don't crash when add-on flips private browsing after document is loaded, for aurora52

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fced93d4a537
https://hg.mozilla.org/releases/mozilla-aurora/rev/3e82bc16778a

Comment 35

[Infocatcher]

Broken in Firefox 51+ (and was patched the beta tree... is this normal to push non-backward compatible changes into stable tree?), but "fixed" only in Firefox 52+.

Also now broken way to open "about:newtab" in new private tab: at "TabOpen" stage it's not possible anymore to set .usePrivateBrowsing flag. Works for _some_ tabs, but not for all.
Any API to handle tab's docShell before initialization?

Also: how to toggle private state of already opened tab? Duplicate tab and close original one?

Comment 36

[Murz]

Please backport patch for 52+ to 51 version, Private Tab is very useful functional!

Comment 37

[Mehdi]

Infocatcher and his awesome extension are waiting for your attention guys, Private Tab is very useful.

Comment 38

[Infocatcher]

(In reply to Murz from comment #36)
> Please backport patch for 52+ to 51 version, Private Tab is very useful
> functional!

This patch isn't enough: it fixes crashes, yes, but also "fixes" feature to open new empty private tab and to toggle private state of already opened tab.

Comment 39

[Kris Maglione [:kmag]]

(In reply to Infocatcher from comment #38)
> This patch isn't enough: it fixes crashes, yes, but also "fixes" feature to
> open new empty private tab and to toggle private state of already opened tab.

Toggling the private browsing state of an existing tab is not supported, and was never meant to be possible. The closest you can come to safely supporting something like that is to replace the tab with a private one, and re-load its session store data.

(In reply to Infocatcher from comment #35)
> Also now broken way to open "about:newtab" in new private tab: at "TabOpen"
> stage it's not possible anymore to set .usePrivateBrowsing flag. Works for
> _some_ tabs, but not for all.

The new tab page is generally pre-loaded into a hidden <browser> element. You'd need to disable preloading if you want to be able to make arbitrary new tabs private.

Comment 40

[Infocatcher]

(In reply to Kris Maglione [:kmag] from comment #39)
> The closest you can come to safely supporting something like that is to
> replace the tab with a private one, and re-load its session store data.

Tab will flicker a bit, but looks doable:
https://github.com/Infocatcher/Private_Tab/commit/891357733c6acb6bf50356517fb1a21ab8f59330


> The new tab page is generally pre-loaded into a hidden <browser> element.
> You'd need to disable preloading if you want to be able to make arbitrary
> new tabs private.

Oh, I see, corrected:
https://github.com/Infocatcher/Private_Tab/commit/a2fd890ab548e7f0251510268e72d5841628f4d7
(yes, unfortunately tricky)

Comment 41

[Paul Silaghi, QA [:pauly]]

(In reply to Infocatcher from comment #35)
> Broken in Firefox 51+ (and was patched the beta tree... is this normal to
> push non-backward compatible changes into stable tree?), but "fixed" only in
> Firefox 52+.
Marking this as verified fixed by Infocatcher in Fx 52

Comment 42

[maxim]

Hi everybody.

Sorry for jumping in a bit too late. I'm Max and I work on CLIQZ Browser (based on Firefox) on a feature similar to what "Private Tab" extension provides, but implemented at browser code level.
This change just hit us as we merged with version 52. It's of course easy to just disable the CanSetOriginAttributes() check and proceed, but I'm truly curious about the security implications here, as we planned to actually try to upstream code for private tabs to Mozilla.
Could you please go into details on how exactly changing the OA harms security? General notes or pointers to design documentation for DocShell et al are also highly appreciated.

TIA,
Max.