565338 - Private browsing test failures when running Firefox in XULRunner mode

Status: RESOLVED FIXED

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

Description

[Drew Willcoxon :adw]

Attachment: log

Running Firefox in XULRunner mode like this:

  cfx -b /Applications/Minefield.app/Contents/MacOS/firefox-bin test

(or with Namoroka) causes the private browsing test to fail with several errors, which I've attached.  (The PB errors start 7 or 8 lines in.)

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

I noticed this once too, but I don't know why... Are we doing something funky when we try to use Firefox for XULRunner?

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

I guess getting the PB service doesn't fail when using Firefox as XULRunner, but using the service does. I wouldn't have expected things in browser to be used, but I guess since it's just in the components dir, it gets loaded? I don't know enough about this though.

Perhaps it would be better to just use xul-app to check if we're in Firefox as opposed to catching when getting the service? I assume that reports correctly in this case. We'd just have to keep updating that check as other apps implement it.

Comment 3

[Atul Varma [:atul]]

Yeah, this is an unfortunate aspect of using Firefox as XULRunner, as far as I can tell. :(  I think that just using xul-app would probably be easiest, although not the most ideal--but the most ideal would probably involve making changes to platform code that allow an XPCOM component to tell a client if it's actually able to function properly in the current environment or something, it seems.

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #3)
> Yeah, this is an unfortunate aspect of using Firefox as XULRunner, as far as I
> can tell. :(  I think that just using xul-app would probably be easiest,
> although not the most ideal

Ok, I'll just do that then.

> but the most ideal would probably involve making
> changes to platform code that allow an XPCOM component to tell a client if it's
> actually able to function properly in the current environment or something, it
> seems.

So it turns out that's actually possible, by having a registerSelf method on the component. See http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserContentHandler.js#900 for an example. It just doesn't seem to be common practice to do so. Probably because Firefox as XULRunner is just an afterthought. I expect problems like this will become more of an issue unless we consistently check xul-app for Firefox instead of checking for functionality :(

Comment 5

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1

check for Firefox at require time instead of try/catch

Comment 6

[(no longer active)]

This approach seems really bad to me.  There is nothing private browsing related in registerSelf quoted in comment 4.  Additionally, the only reason that setting privateBrowsingEnabled could throw NS_ERROR_FAILURE is that if the method is being re-entered, or _currentStatus is not set to idle for some reason, which doesn't seem to be the case here.  <http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js#493>

I think that attachment 444982 [details] [diff] [review] just masks the real issue by not running the tests.  Isn't that right?

Comment 7

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #6)
> This approach seems really bad to me.  There is nothing private browsing
> related in registerSelf quoted in comment 4.

No there isn't. What I meant is that the private browsing component (and pretty much every component in /browser/components) should be using registerSelf to prevent itself from registering when not in Firefox, like the example.

> Additionally, the only reason
> that setting privateBrowsingEnabled could throw NS_ERROR_FAILURE is that if the
> method is being re-entered, or _currentStatus is not set to idle for some
> reason, which doesn't seem to be the case here. 
> <http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js#493>

So what I'm assuming is happening, is that somewhere up the chain things aren't working because we're not actually in Firefox, even though the components think we are.

So for example, we set privateBrowsingEnabled = true, then privateBrowsingEnabled = false.

I think we might get stuck here: http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js#280
sessionstore won't send sessionstore-browser-state-restored because we're not in a browser so it's confused because things it expects don't work, but it swallows errors (maybe?). So PB never transitions out of STATE_IDLE and then we throw.

> I think that attachment 444982 [details] [diff] [review] just masks the real issue by not running the
> tests.  Isn't that right?

Well, yes. The real issue is that the private browsing component shouldn't be registered in XULRunner. But it is when using Firefox as XULRunner since everything in components gets registered.

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #7)
> I think we might get stuck here:
> http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js#280
> sessionstore won't send sessionstore-browser-state-restored because we're not
> in a browser so it's confused because things it expects don't work, but it
> swallows errors (maybe?). So PB never transitions out of STATE_IDLE and then we
> throw.

To be clear, I don't know if that's exactly what's happening, but I'm relatively certain it's something like that. Since the sessionstore-browser-state-restored topic is the only way _notifyIfTransitionComplete gets called, it makes sense.

Comment 9

[(no longer active)]

(In reply to comment #7)
> (In reply to comment #6)
> > This approach seems really bad to me.  There is nothing private browsing
> > related in registerSelf quoted in comment 4.
> 
> No there isn't. What I meant is that the private browsing component (and pretty
> much every component in /browser/components) should be using registerSelf to
> prevent itself from registering when not in Firefox, like the example.

Ah, OK.  If you want that behavior, you could file bugs for each component.  However, let's not jump to conclusions before we actually know what's happening.

> > Additionally, the only reason
> > that setting privateBrowsingEnabled could throw NS_ERROR_FAILURE is that if the
> > method is being re-entered, or _currentStatus is not set to idle for some
> > reason, which doesn't seem to be the case here. 
> > <http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js#493>
> 
> So what I'm assuming is happening, is that somewhere up the chain things aren't
> working because we're not actually in Firefox, even though the components think
> we are.
> 
> So for example, we set privateBrowsingEnabled = true, then
> privateBrowsingEnabled = false.
> 
> I think we might get stuck here:
> http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js#280
> sessionstore won't send sessionstore-browser-state-restored because we're not
> in a browser so it's confused because things it expects don't work, but it
> swallows errors (maybe?). So PB never transitions out of STATE_IDLE and then we
> throw.

That's possible.  Maybe some sessionstore gurus can confirm that.  But in the mean time, try setting the browser.privatebrowsing.keep_current_session pref to true inside the test before the first time you enter the private browsing mode, and see if that makes a difference.

> > I think that attachment 444982 [details] [diff] [review] [details] just masks the real issue by not running the
> > tests.  Isn't that right?
> 
> Well, yes. The real issue is that the private browsing component shouldn't be
> registered in XULRunner. But it is when using Firefox as XULRunner since
> everything in components gets registered.

Like I said about, this can be fixed in Firefox, if we actually make sure that it is what's happening.

(In reply to comment #8)
> (In reply to comment #7)
> > I think we might get stuck here:
> > http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js#280
> > sessionstore won't send sessionstore-browser-state-restored because we're not
> > in a browser so it's confused because things it expects don't work, but it
> > swallows errors (maybe?). So PB never transitions out of STATE_IDLE and then we
> > throw.
> 
> To be clear, I don't know if that's exactly what's happening, but I'm
> relatively certain it's something like that. Since the
> sessionstore-browser-state-restored topic is the only way
> _notifyIfTransitionComplete gets called, it makes sense.

Yes, your theory is at least plausible (and I think it's likely in fact).  Setting that pref will make the private browsing service to completely ignore sessionstore stuff (we set it for xpcshell tests, for example), so if this is the issue, that pref should fix it.  Could you try that?

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 444982 [details] [diff] [review]
Patch v0.1

I'm going to unset this review request for now while Paul and Ehsan work out the approach here.

Paul: feel free to rerequest if this turns out to be the right (or just right now) solution!

Comment 11

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #9)
> Ah, OK.  If you want that behavior, you could file bugs for each component. 
> However, let's not jump to conclusions before we actually know what's
> happening.

So looks like the movement has already started. Bug 553070 is adding an easy way besides registerSelf and implementing it for nsBrowserGlue. I'll go through and file bugs as appropriate on other things in browser/components

> > I think we might get stuck here:
> > http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js#280
> > sessionstore won't send sessionstore-browser-state-restored because we're not
> > in a browser so it's confused because things it expects don't work, but it
> > swallows errors (maybe?). So PB never transitions out of STATE_IDLE and then we
> > throw.
> 
> That's possible.  Maybe some sessionstore gurus can confirm that.

Ohai, that's me. I can't confirm, but I still highly suspect. It'd take a bit more work to confirm.

> > > I think that attachment 444982 [details] [diff] [review] [details] [details] just masks the real issue by not running the
> > > tests.  Isn't that right?
> > 
> > Well, yes. The real issue is that the private browsing component shouldn't be
> > registered in XULRunner. But it is when using Firefox as XULRunner since
> > everything in components gets registered.
> 
> Like I said about, this can be fixed in Firefox, if we actually make sure that
> it is what's happening.

It looks like it is. Comments in bug 553070 support that.

> Yes, your theory is at least plausible (and I think it's likely in fact). 
> Setting that pref will make the private browsing service to completely ignore
> sessionstore stuff (we set it for xpcshell tests, for example), so if this is
> the issue, that pref should fix it.  Could you try that?

Tried and that makes all the difference. That's another possible temporary fix, but not so awesome. I'd prefer my current approach to wallpaper this.

I'll file a new meta bug for Jetpack to track any that popup, which will be blocked by bugs in the appropriate Firefox components. In the meantime, I think it's best to squash this now since I expect there will be more people using Firefox as XULRunner than installing XULRunner.

Comment 12

[(no longer active)]

(In reply to comment #11)
> > Yes, your theory is at least plausible (and I think it's likely in fact). 
> > Setting that pref will make the private browsing service to completely ignore
> > sessionstore stuff (we set it for xpcshell tests, for example), so if this is
> > the issue, that pref should fix it.  Could you try that?
> 
> Tried and that makes all the difference. That's another possible temporary fix,
> but not so awesome. I'd prefer my current approach to wallpaper this.

In that case, it's sessionstore which actually doesn't work in xulrunner mode.

Anyway, I think this is actually a much better temporary fix, since you would still be running some sort of PB tests for Jetpack.  Your approach is basically equivalent to 'hg rm'-ing the test!  ;-)

Comment 13

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #12)
> Anyway, I think this is actually a much better temporary fix, since you would
> still be running some sort of PB tests for Jetpack.  Your approach is basically
> equivalent to 'hg rm'-ing the test!  ;-)

Well not really. It's still running all of the tests for when PB actually works (read: Firefox). On XULRunner there shouldn't be any PB mode, so in that case we would run a test that covers the fake PB-mode.

Your fix runs the tests on a platform where PB isn't supposed to exist. So we fix one problem now, but there could be others that pop up. Not only that, but it only fixes the test, not the actual module, which will still causes things to be weird when used (since it is still possible to use Firefox as XULRunner in a normal capacity). So I really don't think it's a better solution.

Comment 14

[(no longer active)]

Oh, OK, so if the tests actually run when running the jetpack sdk test suite, I stand corrected.

Comment 15

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 444982 [details] [diff] [review]
Patch v0.1

Ehsan and I have worked out our problems, and this seems to be the best way to fix it, for now.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 444982 [details] [diff] [review]
Patch v0.1

Seems perfectly reasonable, r=myk!

Comment 17

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Pushed http://hg.mozilla.org/labs/jetpack-sdk/rev/600b94546bb1

I'll start filing bugs on components as appropriate to get the fix(es) in on the browser side.

Comment 18

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".