1294199 - Add-on sdk/panel thows exception and is blank when show is called in Private Browsing mode

Status: VERIFIED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[screenwise.browsers]

Attachment: Screenshot from 2016-08-10 11:54:47.png

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/52.0.2743.82 Safari/537.36

Steps to reproduce:

Installed add-on.
Opened a private browsing window. 
Clicked on the toolbar icon for our add-on to show the panel.


Actual results:

The panel appeared but was blank (there was no html displayed).
Furthermore, the panel remained broken in regular browsing mode and did not function normally.

It appears that an exception is being thrown during the panel.show() call. As best as I can tell the swapFrameLoader method in sdk/frame/utils:92 is where the exception is originating. Here is what was in the console output:

Exception { message: "", result: 2147500033, name: "NS_ERROR_NOT_IMPLEMENTED", filename: "resource://gre/modules/commonjs/too…", lineNumber: 92, columnNumber: 0, data: null, stack: "swapFrameLoaders@resource://gre/mod…", location: XPCWrappedNative_NoHelper }

This happens on 49+, my most recent tests were on 51 (Nightly).
48 and below work normally.


Expected results:

The panel should have displayed text based on the fact that the user is in private browsing mode. When the user goes back to regular browsing mode, the panel should show different text indicating they are in regular mode and work normally.

Comment 1

[screenwise.browsers]

The Panel exception effects the content script that is running so it no longer responds to port.on messages. This is why the state of the popup html remains broken and unchanging.

Comment 2

[Tooru Fujisawa [:arai]]

Can you provide a testcase addon?

Comment 3

[screenwise.browsers]

Attachment: Testcase Addon

Here is a simple addon that exhibits similar behavior. The two differences are:

1) For the testcase, the exception also states that the textArea is null while ours gives no indication of anything being null. Instead ours simply prints out the exception with the swapFrameLoaders error.

2) For our extension, the content script seems to quit listening to port events while this extension actually continues to respond to the

Thanks!

Comment 4

[screenwise.browsers]

(In reply to screenwise.browsers from comment #3)
> Created attachment 8781245 [details]
> Testcase Addon
> 
> Here is a simple addon that exhibits similar behavior. The two differences
> are:
> 
> 1) For the testcase, the exception also states that the textArea is null
> while ours gives no indication of anything being null. Instead ours simply
> prints out the exception with the swapFrameLoaders error.
> 
> 2) For our extension, the content script seems to quit listening to port
> events while this extension actually continues to respond to the
> 
> Thanks!

#2 should end as "...respond to them."

Comment 5

[screenwise.browsers]

Looks like it is similar to this bug reported on 6/9:

https://bug98304.bugzilla.mozilla.org/show_bug.cgi?id=1283342

Comment 6

[Tooru Fujisawa [:arai]]

Thanks.

Here's regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=16b4946069a9470a94f33791de7440966844747d&tochange=ff298d2993a32aeaca0eacac32d8e673298cf5c5

Looks like a regression from bug 1269361.

not sure which component this should be handled tho, moving to Add-on SDK for now.

Comment 7

[screenwise.browsers]

Any idea of whether a bug like this will be fixed in 49 before it goes live? This breakage is pretty detrimental to our add on.

Thanks!

Comment 8

[Josh Matthews [:jdm]]

[Tracking Requested - why for this release]: Extensions using the addon SDK to show panels are busted when opened from private windows.

Comment 9

[Josh Matthews [:jdm]]

On the positive side, this is an e10s-only regression, so a significant portion of the Firefox 49 user base will not be affected.

Comment 10

[Josh Matthews [:jdm]]

We're hitting the case in nsFrameLoader::SwapWithOtherLoader where we get the origin attributes of the two docshells and compare them. This looks like another instance of the top level window for the private window using the system principal, while child docshells have the private browsing id origin attribute set. Ehsan, what's the right thing to do here?

Comment 12

[:ehsan akhgari]

Talking to Josh about this on IRC.  Here is the relevant part of the chat:


ehsan
jdm: ok so now I know some things
ehsan
1. we create two iframes
ehsan
2. backgroundFrame is put into addonWindow's doc
ehsan
3. addonWindow is the window obj for a XUL doc living in the hidden window in the parent process
ehsan
4. viewFrame is put into panel's doc, which is the current xul:browser
ehsan
5. we try to swap these frameloaders
ehsan
jdm: now at least the symptom makes sense
jdm
hidden private window?
ehsan
let me check
ehsan
no
ehsan
that is the bug
ehsan
jdm: can you see if fixing this http://searchfox.org/mozilla-central/source/addon-sdk/source/lib/sdk/window/utils.js#64 will make the bug go away?
ehsan
fixing it as in using appShellService.hiddenPrivateDOMWindow if we're in a PB window?
jdm
sure

Comment 13

[Josh Matthews [:jdm]]

It turns out that the modifying the code Ehsan linked does not fix this issue, due to the way the `hiddenWindow` API is used by the rest of the SDK. Specifically, there are various places in the code that get the singular hidden window at startup and use it for all future operations, so they still don't end up using the private hidden window as desired. Fixing all of these uses looks like a significant amount of work, much like the original per-window private browsing rewrite that we worked on back in the day.

On top of that, it turns out that this bug is much less of a risk to our release audiences than originally assumed. There are no plans to enable e10s for users with Addon SDK extensions enabled, so it's only users on the beta, aurora, and nightly channels that will encounter this issue. Additionally, since this bug only affects panels opened from private windows, the workaround for users is to use the panel from a non-private window. Accordingly, I'm going to deem this a soft-WONTFIX. If someone really wants to put in the effort to fix this properly I'll cheer them on, but I think there are more important things to work on.

Comment 14

[:ehsan akhgari]

screenwise.browsers, FWIW I suggest you look at WebExtensions <https://wiki.mozilla.org/WebExtensions> and see if you can start porting your add-on to that API.  Thankfully the port can be done in a piecemeal fashion, so for example you may have some luck using the WebExtensions panel API instead of the add-on SDK panel API which will hopefully not have this issue.  See <https://wiki.mozilla.org/Add-ons/developer/communication#Migration_Path>.  Without that, your add-on ASK based extension will force the user to not use multi-process Firefox, which will affect the performance and stability of Firefox for those users.

Comment 15

[screenwise.browsers]

Ehsan,

Yeah, we are looking into WebExtensions but haven't been able to start that yet. Our add-on is pretty extensive and that will take some time. I do like the fact that we can do it in piecemeal though (I haven't had any luck with converting the panel API use yet, but I've only started playing around with that).

As for multi-process, we have upgraded the add-on to be multi-process compatible and have successfully tested in. Am I missing something wrt how/when multi-process FF is turned on for users?

Thanks!

Comment 16

[screenwise.browsers]

I see that it has been marked as 'wontfix' for 49. However, is there any plan to fix it in later versions?

Comment 17

[Josh Matthews [:jdm]]

I am not aware of any plans to fix this, since it will not affect release versions of Firefox for the forseeable future.

Comment 18

[Kris Maglione [:kmag]]

We should probably just handle this by making the browser in the panel non-private before we swap the frame loaders, even if it's in a private window. It's not ideal, but it's the behavior that we've had all along.

Comment 19

[:ehsan akhgari]

(In reply to Kris Maglione [:kmag] from comment #18)
> We should probably just handle this by making the browser in the panel
> non-private before we swap the frame loaders, even if it's in a private
> window. It's not ideal, but it's the behavior that we've had all along.

It's more involved than that, since either of the two windows involved may or may not be private.  I'm pretty sure that for example in the case where you turn on permanent private browsing, the hidden window will be private too, so your suggestion will cause the same issue in that case.  I doubt this can be fixed without making the entire code that deals with the hidden window aware of private browsing.

Comment 20

[:ehsan akhgari]

(In reply to screenwise.browsers from comment #15)
> Ehsan,
> 
> Yeah, we are looking into WebExtensions but haven't been able to start that
> yet. Our add-on is pretty extensive and that will take some time. I do like
> the fact that we can do it in piecemeal though (I haven't had any luck with
> converting the panel API use yet, but I've only started playing around with
> that).

Sounds good!

> As for multi-process, we have upgraded the add-on to be multi-process
> compatible and have successfully tested in. Am I missing something wrt
> how/when multi-process FF is turned on for users?

Your add-on is multi-process incompatible by the virtue of using the add-on SDK in the first place.  :-)  When Firefox is trying to determine whether it can turn off multi-process for a user, it looks at a number of conditions including the installed extensions and will currently bail out if an extension is installed.  I think the e10s team is working to make the checks more relaxed to allow for a white-list of add-ons that we have manually verified to work well with e10s.

When I spoke to someone on the team, they said that there is currently no plans on supporting add-on SDK Based add-ons in multi-process Firefox, which is why we decided that it's not worth fixing this bug.

Last but not least, note that the behavior of Firefox on different branches is different.  For example, on beta, we allow e10s to be turned on if you have an extension such as the ones I described above installed, which is why you experienced the bug in the first place, but if you tested your extension on Firefox 49 release, then the bug wouldn't show up since the presence of your extension would force Firefox out of the e10s mode, which would make everything work fine...

Comment 22

[Loic]

(In reply to Josh Matthews [:jdm] from comment #9)
> On the positive side, this is an e10s-only regression, 

Are you sure about that? Because I used a custom profile with e10s disabled and add-on Video DownloadHelper installed to reproduce the issue and find the regressing bug.

Many add-ons are affected like VDH, LastPass, ZenMate VPN, etc.

Comment 23

[Kev Needham [:kev]]

(In reply to Loic from comment #22)
> (In reply to Josh Matthews [:jdm] from comment #9)
> > On the positive side, this is an e10s-only regression, 
> 
> Are you sure about that? Because I used a custom profile with e10s disabled
> and add-on Video DownloadHelper installed to reproduce the issue and find
> the regressing bug.

Concur. See this in non-e10s Firefox as well, and if you open a panel in private browsing and close private browsing, the behaviour is seen in non-private browsing tabs as well until a restart.

Comment 24

[Loic]

IMHO, tracking for 49 should be reconsidered. There are already 2 dupes, and probably more will be added if 49 is going to be released with this regression.

Comment 25

[Jorge Villalobos [:jorgev] (he/him)]

Do we have a good idea of what an add-on needs to do to run into this bug? I can confirm that this doesn't depend on e10s, but I see some add-ons working (https://addons.mozilla.org/addon/youtube-panel/), so it's not affecting all using panels on Private Mode.

Comment 26

[:ehsan akhgari]

Note that bug 1269361 was backed out from Firefox 49.  If this regression affects more than e10s, we should fix it for Firefox 50.

Comment 27

[:ehsan akhgari]

(In reply to Jorge Villalobos [:jorgev] from comment #25)
> Do we have a good idea of what an add-on needs to do to run into this bug? I
> can confirm that this doesn't depend on e10s, but I see some add-ons working
> (https://addons.mozilla.org/addon/youtube-panel/), so it's not affecting all
> using panels on Private Mode.

I don't know.  Josh was the person who investigated this, not me.

Comment 28

[Liz Henry (:lizzard)]

We can keep tracking it for 49 and ask for more testing here for the RC2 build. 
Shell is this something you can help coordinate?  Andrei's team or do you have separate testing/QA for addons?

Comment 29

[Liz Henry (:lizzard)]

Josh, ehsan suggested I ask you for opinions on this issue. So I'm inviting you to the needinfo party. Which addons might be affected, what should we test other than the ones mentioned in comment 22?

Comment 30

[Kris Maglione [:kmag]]

I'm fairly certain this was caused by either bug 1242644 or bug 1260766, so 49 should still be affected.

(In reply to Jorge Villalobos [:jorgev] from comment #25)
> Do we have a good idea of what an add-on needs to do to run into this bug? I
> can confirm that this doesn't depend on e10s, but I see some add-ons working
> (https://addons.mozilla.org/addon/youtube-panel/), so it's not affecting all
> using panels on Private Mode.

That add-on creates its own panel rather than using the sdk/panel module.

Comment 31

[Loic]

VideoDownload Helper is affected.
I think this simple add-on displaying server name of the website too.
https://addons.mozilla.org/en-US/firefox/addon/server-view/

Comment 32

[:ehsan akhgari]

(In reply to Kris Maglione [:kmag] from comment #30)
> I'm fairly certain this was caused by either bug 1242644 or bug 1260766, so
> 49 should still be affected.

Comment 6 seems to disagree, but at this point we should just test!  There's no reason to guess.

Comment 33

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari from comment #32)
> (In reply to Kris Maglione [:kmag] from comment #30)
> > I'm fairly certain this was caused by either bug 1242644 or bug 1260766, so
> > 49 should still be affected.
> 
> Comment 6 seems to disagree, but at this point we should just test!  There's
> no reason to guess.

Sorry, you're right. It's the combination of the two that causes it.

Comment 34

[Andy McKay]

We believe this was caused by bug 1297687 comment 43 which has been backed out of 49. When RC2 is built, let's double check this is ok. If it is, then we can focus on the patch for 50.

Comment 35

[Andy McKay]

I was able to reproduce the problem on nightly.

I then downloaded a build https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-beta-macosx64/1473198616/ from build https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&selectedJob=1597652 and was NOT able to reproduce the problem.

We'll double check with RC2 and remove the status-firefox 49 then.

Comment 36

[:shell escalante]

(In reply to :Ehsan Akhgari from comment #20)

> Last but not least, note that the behavior of Firefox on different branches
> is different.  For example, on beta, we allow e10s to be turned on if you
> have an extension such as the ones I described above installed, which is why
> you experienced the bug in the first place, but if you tested your extension
> on Firefox 49 release, then the bug wouldn't show up since the presence of
> your extension would force Firefox out of the e10s mode, which would make
> everything work fine...

just some details: 
49 Release will allow users with only webextensions or 49 Beta experiment named add-ons.  There are 10, including VHD.  We have the capability to remove a named add-on from being eligible for e10s if needed.  

Beta Experiment add-ons: greasemonkey, AdBlock Plus, Video Download helper, uBlock Origin, Download YouTube Videos as MP4, Lightbeam, Awesome Screenshot Plus, Emoji Cheatsheet, Persona Plus, Ad-on Compatibility Reporter

Comment 37

[:ehsan akhgari]

I just realized this isn't marked for tracking past 49.  If the backout proves to have fixed it in 49, we need to start to track this for 50 and 51 for the same reasons as before.

Comment 39

[Kris Maglione [:kmag]]

Attachment: Bug 1294199: Part 1 - Add a docShell flag to allow legacy panels to skip inheriting origin attributes.

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

Comment 40

[Kris Maglione [:kmag]]

Attachment: Bug 1294199: Part 2 - Fix rendering of SDK panels in private browsing windows.

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

Comment 41

[Josh Matthews [:jdm]]

It sounds like any addon that uses the SDK panel API and has the private-browsing permission enabled in its package.json can trigger this bug.

Comment 42

[fdsc]

I want to note that even before the e10s has been a similar bug. It has not been fixed.
When closing private Windows, if the panel was visible, the panel has gone altogether in any windows about the same as now.

Comment 43

[Marcia Knous [:marcia - needinfo? me/PTO 12/6-12/12)]]]

Tracking 50/51+ - as Ehsan notes in Comment 37 we should keep tracking this.

Comment 44

[:ehsan akhgari]

Comment on attachment 8789592 [details]
Bug 1294199: Part 1 - Add a docShell flag to allow legacy panels to skip inheriting origin attributes.

https://reviewboard.mozilla.org/r/77750/#review76688

::: docshell/base/nsIDocShell.idl:305
(Diff revision 1)
>     */
>    [infallible] attribute boolean allowContentRetargetingOnChildren;
>  
>    /**
> +   * True if this docShell should inherit origin and security attributes from
> +   * its parent when reparented.

I'm not sure if it's a good idea to make this flag control anything more than the PB status, unless there is a specific reason why not inheriting other things is desired here?

::: docshell/base/nsIDocShell.idl:310
(Diff revision 1)
> +   * its parent when reparented.
> +   *
> +   * NOTE: This should *not* be set false in new code, or for docShells
> +   * inserted anywhere other than as children of panels.
> +   */
> +  [infallible] attribute boolean inheritOriginAttributes;

This name is misleading, since we don't really inherit the OAs from the parent; it's just the private browsing bit.  Maybe use inheritPrivateBrowsingId?

Comment 45

[:ehsan akhgari]

Also, I think it would be a good idea to also ask bz's review on the next iteration of the docshell patch.

Thanks for looking into this!

Comment 46

[Kris Maglione [:kmag]]

Comment on attachment 8789592 [details]
Bug 1294199: Part 1 - Add a docShell flag to allow legacy panels to skip inheriting origin attributes.

https://reviewboard.mozilla.org/r/77750/#review76688

> I'm not sure if it's a good idea to make this flag control anything more than the PB status, unless there is a specific reason why not inheriting other things is desired here?

I'm just trying to head off other potential problems related to moving the panel between windows. I don't think any of the other inherited flags currently make a difference one way or the other, aside from `affectsPrivateSessionLifetime`, because we don't set any of them per-browser-window. But if that changes at some point, and we move the panel to a window where it inherits, say, a custom user agent string, which sticks around when it gets moved to another window, it's almost certainly going to cause confusing issues.

> This name is misleading, since we don't really inherit the OAs from the parent; it's just the private browsing bit.  Maybe use inheritPrivateBrowsingId?

Well, at the moment it's just the private browsing ID, but how confident are we that that won't change? Anyway, my purpose for this flag is to allow docshells to be swapped between windows without errors from origin attribute mismatches, so the name is more about the goal than the specifics of the implementation.

I'm OK with the name change if we drop the changes to inheritance of other properties, though.

Comment 47

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari from comment #45)
> Also, I think it would be a good idea to also ask bz's review on the next
> iteration of the docshell patch.

It looks like bz isn't currently accepting review requests.

Comment 48

[Andy McKay]

Confirmed, seems to be fixed in RC2. Taking off tracking for 49.

Comment 49

[:ehsan akhgari]

(In reply to Kris Maglione [:kmag] from comment #46)
> Comment on attachment 8789592 [details]
> Bug 1294199: Part 1 - Add a docShell flag to allow legacy panels to skip
> inheriting origin attributes.
> 
> https://reviewboard.mozilla.org/r/77750/#review76688
> 
> > I'm not sure if it's a good idea to make this flag control anything more than the PB status, unless there is a specific reason why not inheriting other things is desired here?
> 
> I'm just trying to head off other potential problems related to moving the
> panel between windows. I don't think any of the other inherited flags
> currently make a difference one way or the other, aside from
> `affectsPrivateSessionLifetime`, because we don't set any of them
> per-browser-window. But if that changes at some point, and we move the panel
> to a window where it inherits, say, a custom user agent string, which sticks
> around when it gets moved to another window, it's almost certainly going to
> cause confusing issues.

That's fair.  But I still think we should not do that work in this patch.  Especially since this needs to be backported to beta (Firefox 50).  Doing the rest in a different bug would make sense to me (assuming Boris is OK with it.)

> > This name is misleading, since we don't really inherit the OAs from the parent; it's just the private browsing bit.  Maybe use inheritPrivateBrowsingId?
> 
> Well, at the moment it's just the private browsing ID, but how confident are
> we that that won't change?

For the most part, OAs are attributes of the origin that you're loading, so inheriting them across docshells (which can have arbitrary documents loaded inside them) doesn't make much sense.  PB is the exception to this rule.

> Anyway, my purpose for this flag is to allow
> docshells to be swapped between windows without errors from origin attribute
> mismatches, so the name is more about the goal than the specifics of the
> implementation.
> 
> I'm OK with the name change if we drop the changes to inheritance of other
> properties, though.

I think the way forward could be:

* Only deal with PB in this patch.
* Land it and get it backported and whatnot.
* File a follow-up to handle the rest of the cases which are inherited from the parent, and fix that separately.

What do you think?

(In reply to Kris Maglione [:kmag] from comment #47)
> (In reply to :Ehsan Akhgari from comment #45)
> > Also, I think it would be a good idea to also ask bz's review on the next
> > iteration of the docshell patch.
> 
> It looks like bz isn't currently accepting review requests.

If your patch ends up only dealing with the PB ID, I can review it comfortably.  Otherwise another reviewer who you can try is smaug.  My comment here was mostly because this patch was modifying non-PB-related parts of the docshell.

Comment 50

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari from comment #49)
> I think the way forward could be:
> 
> * Only deal with PB in this patch.
> * Land it and get it backported and whatnot.
> * File a follow-up to handle the rest of the cases which are inherited from
> the parent, and fix that separately.
> 
> What do you think?

That sounds good to me.

Comment 51

[:ehsan akhgari]

Comment on attachment 8789592 [details]
Bug 1294199: Part 1 - Add a docShell flag to allow legacy panels to skip inheriting origin attributes.

https://reviewboard.mozilla.org/r/77750/#review76784

Comment 52

[Kris Maglione [:kmag]]

Comment on attachment 8789592 [details]
Bug 1294199: Part 1 - Add a docShell flag to allow legacy panels to skip inheriting origin attributes.

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

Comment 53

[Kris Maglione [:kmag]]

Comment on attachment 8789593 [details]
Bug 1294199: Part 2 - Fix rendering of SDK panels in private browsing windows.

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

Comment 54

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8789593 [details]
Bug 1294199: Part 2 - Fix rendering of SDK panels in private browsing windows.

https://reviewboard.mozilla.org/r/77752/#review77250

Thanks, I slowly diggested all the changes finally I think. Sorry for this taking so long but it's been a while since I looked at this code and I won't say that I wasn't trying to forget this peace of beauty. I have a small performance concern with this approach but probably it's fine, so r+ modulo how the 1st patch gets sorted out.

::: addon-sdk/source/lib/sdk/panel/utils.js:190
(Diff revision 2)
> +  panel.viewFrame = document.importNode(panel.backgroundFrame, false);
> +  panel.appendChild(panel.viewFrame);
> +
> +  let principal = Services.scriptSecurityManager.createNullPrincipal({});
> +  getDocShell(panel.viewFrame).createAboutBlankContentViewer(principal);

I have little performance concern about doing this here... but if it works out smoothly in practice then I have no problem with it.

Comment 55

[Kris Maglione [:kmag]]

Comment on attachment 8789593 [details]
Bug 1294199: Part 2 - Fix rendering of SDK panels in private browsing windows.

https://reviewboard.mozilla.org/r/77752/#review77250

> I have little performance concern about doing this here... but if it works out smoothly in practice then I have no problem with it.

It doesn't seem to make any noticeable difference, as far as I've been able to see. I think the performance is probably about the same as, or slightly better than, the old code except in the case where the panel isn't being moved between windows. Possibly slightly different when opening twice in the same window, but at least we have the benefit of not having an extra docShell hanging around in-between.

In any case, I can't think of a better option. We can't change the origin attributes if anything other than about:blank has been loaded into the docShell (which means we can't load an initial `data:` URL like we did previously), and we can't swap the docshells until a content viewer has been created.

Comment 56

[:ehsan akhgari]

Comment on attachment 8789592 [details]
Bug 1294199: Part 1 - Add a docShell flag to allow legacy panels to skip inheriting origin attributes.

https://reviewboard.mozilla.org/r/77750/#review77602

::: docshell/base/nsDocShell.cpp:3405
(Diff revision 2)
>  
>    nsCOMPtr<nsILoadContext> parentAsLoadContext(do_QueryInterface(parent));
> -  if (parentAsLoadContext &&
> +  if (parentAsLoadContext && mInheritPrivateBrowsingId &&
>        NS_SUCCEEDED(parentAsLoadContext->GetUsePrivateBrowsing(&value))) {
>      SetPrivateBrowsing(value);
>    }

So at this point, the PB-ness of this docshell should not have changed.  Can you please add some assertion to ensure that the value of UsePrivateBrowsing() won't change from the beginning of the function to after this branch if mInheritPrivateBrowsungId is false?

::: docshell/base/nsIDocShell.idl:304
(Diff revision 2)
>     * Setting allowContentRetargeting also overwrites this value.
>     */
>    [infallible] attribute boolean allowContentRetargetingOnChildren;
>  
>    /**
> +   * True if this docShell should inherit the private browsing context from

Nit: s/context/ID/

Comment 57

[:ehsan akhgari]

Comment on attachment 8789592 [details]
Bug 1294199: Part 1 - Add a docShell flag to allow legacy panels to skip inheriting origin attributes.

https://reviewboard.mozilla.org/r/77750/#review77604

Arghhh mozreview.  Sorry!

r=me, this looks great!

Comment 58

[Kris Maglione [:kmag]]

Comment on attachment 8789592 [details]
Bug 1294199: Part 1 - Add a docShell flag to allow legacy panels to skip inheriting origin attributes.

https://reviewboard.mozilla.org/r/77750/#review77602

> So at this point, the PB-ness of this docshell should not have changed.  Can you please add some assertion to ensure that the value of UsePrivateBrowsing() won't change from the beginning of the function to after this branch if mInheritPrivateBrowsungId is false?

Sure, sounds like a good idea.

Comment 59

[Kris Maglione [:kmag]]

Thanks!

Comment 60

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e03f80d882f29de97006bd3ed80da20b52a40ff
Bug 1294199: Part 1 - Add a docShell flag to allow legacy panels to skip inheriting origin attributes. r=ehsan

https://hg.mozilla.org/integration/mozilla-inbound/rev/1430290455ed94109e973fd17c7f62865e1695c9
Bug 1294199: Part 2 - Fix rendering of SDK panels in private browsing windows. r=gabor

Comment 61

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4e03f80d882f
https://hg.mozilla.org/mozilla-central/rev/1430290455ed

Comment 62

[Kris Maglione [:kmag]]

Comment on attachment 8789593 [details]
Bug 1294199: Part 2 - Fix rendering of SDK panels in private browsing windows.

Approval Request Comment
[Feature/regressing bug #]: Bug 1269361
[User impact if declined]: This bug causes panels created by SDK add-ons to malfunction, and show only a blank document, when opened in private browsing windows. Given the number of duplicates already filed, this is affecting a large number of users.
[Describe test coverage new/current, TreeHerder]: The existing tests which should have tested this behavior were previously disabled. They have been re-enabled, and extended to reliably detect regressions.
[Risks and why]: Moderate. The code changes are fairly minimal, and for the most part simply restore the behavior that existed prior to bug 1269361. Some details of the internals of the sdk/panel implementation have changed, however, and may affect add-ons which break its abstraction. I couldn't find any add-on code on DXR which would likely be affected, though.
[String/UUID change made/needed]: None.

(Note: This request is for uplift to beta 50)

Comment 63

[Kris Maglione [:kmag]]

Comment on attachment 8789592 [details]
Bug 1294199: Part 1 - Add a docShell flag to allow legacy panels to skip inheriting origin attributes.

Approval Request Comment
See comment 62

Comment 64

[Ritu Kothari (:ritu)]

Hello there, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 65

[Ritu Kothari (:ritu)]

Comment on attachment 8789593 [details]
Bug 1294199: Part 2 - Fix rendering of SDK panels in private browsing windows.

Fixes a recent regression, Beta50+

Comment 66

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/71073de4c0e0
https://hg.mozilla.org/releases/mozilla-beta/rev/100c26355ce3

Comment 67

[Vasilica Tamas, QA]

I was able to reproduce the initial issue on Firefox 51.0a1 (2016-08-10) under Windows 10 64-bit.

Verified as fixed on Firefox 52.0a1 (2016-09-20), Firefox 51.0a2 (2016-09-21) and Firefox 50.0b1 (20160920155715) under Windows 10 64-bit, Ubuntu 16.04 32-bit and Mac OS X 10.10.4. 

The add-on panel is successfully displayed in Private Window while testing using the testcase from Comment 3, but I’ve noticed a new regression: the panel size increases each time the add-on button is clicked http://screencast.com/t/ipND2rKxsqt for which I’ve file Bug 1304379.

I am marking this bug as Verified since the other issue is tracked separately.

Comment 68

[Emmanuel SELLIER]

It seems there are collateral issues...
on Firefox 51.0a2 (2016-09-30) (64 bits) MacOS Sierra 10.12.1
The panel content is not correctly displayed as it is (same code) with Firefox 48.0.1

See screen captures :
With Firefox 48.0.1 : https://extension.verifrom.com/extension/bugMoz/FF48.png
With Firefox 51.0a2 : https://extension.verifrom.com/extension/bugMoz/FF51.png

Maybe my fault... in the HTML content of the panel.
Still, something changed from the past.

Comment 69

[Emmanuel SELLIER]

(In reply to Emmanuel SELLIER from comment #68)
> It seems there are collateral issues...
> on Firefox 51.0a2 (2016-09-30) (64 bits) MacOS Sierra 10.12.1
> The panel content is not correctly displayed as it is (same code) with
> Firefox 48.0.1
> 
> See screen captures :
> With Firefox 48.0.1 :
> https://extension.verifrom.com/extension/bugMoz/FF48.png
> With Firefox 51.0a2 :
> https://extension.verifrom.com/extension/bugMoz/FF51.png
> 
> Maybe my fault... in the HTML content of the panel.
> Still, something changed from the past.

Fixed in my HTML content, I had to specify a width property in style attribute of the HTML tag.

Comment 70

[screenwise.browsers]

(In reply to :Ehsan Akhgari from comment #20)
> (In reply to screenwise.browsers from comment #15)
> 
> Your add-on is multi-process incompatible by the virtue of using the add-on
> SDK in the first place.  :-)  When Firefox is trying to determine whether it
> can turn off multi-process for a user, it looks at a number of conditions
> including the installed extensions and will currently bail out if an
> extension is installed.  I think the e10s team is working to make the checks
> more relaxed to allow for a white-list of add-ons that we have manually
> verified to work well with e10s.
> 
> When I spoke to someone on the team, they said that there is currently no
> plans on supporting add-on SDK Based add-ons in multi-process Firefox, which
> is why we decided that it's not worth fixing this bug.

Hi Ehsan!

Sorry for the delay, but I only just noticed this comment.

I'm confused, you say that e10s doesn't support any add-ons using the add on SDK, but from what I have read I thought that high-level API use of the add-on SDK was supported (https://developer.mozilla.org/en-US/Add-ons/Working_with_multiprocess_Firefox). Plus, I did the testing steps advised to simulate being in multiprocess mode and all appeared to work fine. Has something changed with this so that all add-on SDK use is not going to be multiprocess compatable?

Thanks!