Closed Bug 1294199 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect

Tracking

(firefox48 unaffected, firefox49 unaffected, firefox50+ verified, firefox51+ verified)

VERIFIED FIXED
mozilla51
Tracking Status
firefox48 --- unaffected
firefox49 --- unaffected
firefox50 + verified
firefox51 + verified

People

(Reporter: screenwise.browsers, Assigned: kmag)

References

Details

(Keywords: regression, Whiteboard: triaged)

Attachments

(4 files)

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.
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.
Can you provide a testcase addon?
Flags: needinfo?(screenwise.browsers)
Attached file 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!
Flags: needinfo?(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."
Looks like it is similar to this bug reported on 6/9:

https://bug98304.bugzilla.mozilla.org/show_bug.cgi?id=1283342
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.
Blocks: 1269361
Status: UNCONFIRMED → NEW
QA Whiteboard: [bugday-20160815]
Component: Untriaged → General
Ever confirmed: true
Keywords: regression
Product: Firefox → Add-on SDK
See Also: → 1283342
Version: 49 Branch → unspecified
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!
[Tracking Requested - why for this release]: Extensions using the addon SDK to show panels are busted when opened from private windows.
On the positive side, this is an e10s-only regression, so a significant portion of the Firefox 49 user base will not be affected.
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?
Flags: needinfo?(ehsan)
Duplicate of this bug: 1283342
Blocks: e10s-addons
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
Flags: needinfo?(ehsan)
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.
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.
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!
I see that it has been marked as 'wontfix' for 49. However, is there any plan to fix it in later versions?
I am not aware of any plans to fix this, since it will not affect release versions of Firefox for the forseeable future.
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.
(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.
(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...
Assignee: nobody → kmaglione+bmo
Priority: -- → P1
Whiteboard: triaged
Duplicate of this bug: 1300607
(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.
Flags: needinfo?(josh)
(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.
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.
Flags: needinfo?(lhenry)
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.
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(ehsan)
Note that bug 1269361 was backed out from Firefox 49.  If this regression affects more than e10s, we should fix it for Firefox 50.
(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.
Flags: needinfo?(ehsan)
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?
Flags: needinfo?(sescalante)
Flags: needinfo?(lhenry)
Flags: needinfo?(andrei.vaida)
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?
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.
Flags: needinfo?(kmaglione+bmo)
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/
(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.
(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.
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.
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.
(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
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.
No longer blocks: e10s-addons
Duplicate of this bug: 1301183
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.
Flags: needinfo?(josh)
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.
Tracking 50/51+ - as Ehsan notes in Comment 37 we should keep tracking this.
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?
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 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.
(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.
Confirmed, seems to be fixed in RC2. Taking off tracking for 49.
(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.
(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 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
Attachment #8789592 - Flags: review?(ehsan) → review-
Flags: needinfo?(andrei.vaida)
OS: Unspecified → All
QA Contact: vasilica.mihasca
Hardware: Unspecified → All
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.
Attachment #8789593 - Flags: review?(gkrizsanits) → review+
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 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/
Attachment #8789592 - Flags: review?(ehsan) → review-
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!
Attachment #8789592 - Flags: review- → review+
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.
Thanks!
Flags: needinfo?(sescalante)
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
https://hg.mozilla.org/mozilla-central/rev/4e03f80d882f
https://hg.mozilla.org/mozilla-central/rev/1430290455ed
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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)
Attachment #8789593 - Flags: approval-mozilla-beta?
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
Attachment #8789592 - Flags: approval-mozilla-beta?
Hello there, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(screenwise.browsers)
Comment on attachment 8789593 [details]
Bug 1294199: Part 2 - Fix rendering of SDK panels in private browsing windows.

Fixes a recent regression, Beta50+
Attachment #8789593 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8789592 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1304379
Depends on: 1304381
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(screenwise.browsers)
Depends on: 1305148
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.
(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.
Depends on: 1310350
(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!
You need to log in before you can comment on or make changes to this bug.