855484 - non-private windows open from private windows are ignored by WindowTracker, Widget,...

Status: RESOLVED FIXED

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

Description

[Alexandre Poirot [:ochameau]]

For some reason, nsILoadContext.usePrivateBrowsing ends up being true, for top-level xul windows when we open a new non-private one from a private window...
It seems to be a race condition in platform code, as usePrivateBrowsing gets back to false after a setTimeout 0.

One easy way to fix this is to wait for window load before checking for private-browsing status.

At least WindowTracker and Widget are impacted, but any other API using WindowTracker will suffer from this bug.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 906

We should take a look at why this race happens at platform level,
but in the meantime, here is a fix if we consider this important to fix quickly.

Comment 2

[Erik Vold [:erikvold] (please needinfo? me)]

Damn, it looks like you have another important fix in your pull Alex, which is to check pb state of windows that unload before the add-on unloads..

We'll have to do a hotfix for this I think.

Comment 3

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Alexandre Poirot (:ochameau) from comment #1)
> Created attachment 730372 [details]
> Pull request 906
> 
> We should take a look at why this race happens at platform level,
> but in the meantime, here is a fix if we consider this important to fix
> quickly.

This looks fine, but we need tests.

Comment 4

[Jeff Griffiths (:canuckistani) (:⚡︎)]

We discussed this ( and will discuss it again in triage tomorrow ) but my inclination is to ask for a platform fix and the relevant uplifts to Aurora 22 and Beta 21 when the platform fix lands.

"to check pb state of windows that unload before the add-on unloads" I would need to be convinced somehow this is important enough to hotfix 1.14 for. It might be possible to uplift to Aurora 22 though.

Comment 5

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Jeff Griffiths (:canuckistani) from comment #4)
> We discussed this ( and will discuss it again in triage tomorrow ) but my
> inclination is to ask for a platform fix and the relevant uplifts to Aurora
> 22 and Beta 21 when the platform fix lands.
> 
> "to check pb state of windows that unload before the add-on unloads" I would
> need to be convinced somehow this is important enough to hotfix 1.14 for. It
> might be possible to uplift to Aurora 22 though.

Basically:

WindowTracker({
  onTrack: function() {},
  onUntrack: function(window) {
    // tracks private windows when a private window is closed
    // does not track private windows when the add-on is closed
  }
})

and I think that needs to be fixed because it'll provide access to private windows to and add-on that not have permission to them.  Usually this is used for clean up, so it may not be a big issue, but I'd like to get this fixed if possible.

Comment 6

[Erik Vold [:erikvold] (please needinfo? me)]

It'll be a small patch to uplift if that helps :)

Comment 7

[Erik Vold [:erikvold] (please needinfo? me)]

I'll write the tests now.

Comment 8

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #5)
> (In reply to Jeff Griffiths (:canuckistani) from comment #4)
> > We discussed this ( and will discuss it again in triage tomorrow ) but my
> > inclination is to ask for a platform fix and the relevant uplifts to Aurora
> > 22 and Beta 21 when the platform fix lands.
> > 
> > "to check pb state of windows that unload before the add-on unloads" I would
> > need to be convinced somehow this is important enough to hotfix 1.14 for. It
> > might be possible to uplift to Aurora 22 though.
> 
> Basically:
> 
> WindowTracker({
>   onTrack: function() {},
>   onUntrack: function(window) {
>     // tracks private windows when a private window is closed
>     // does not track private windows when the add-on is closed
>   }
> })
> 
> and I think that needs to be fixed because it'll provide access to private
> windows to and add-on that not have permission to them.  Usually this is
> used for clean up, so it may not be a big issue, but I'd like to get this
> fixed if possible.

Ah WindowTracker is fine, I was wrong above :)

I have a test for this case here https://github.com/mozilla/addon-sdk/blob/master/test/test-window-utils-private-browsing.js#L25 and it works.  No uplift needed, just the platform fix would suffice.

Do we have a bug for that?

Comment 9

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 730372 [details]
Pull request 906

This is a fine work around for the sdk, but a platform fix should be made too.

Comment 10

[Alexandre Poirot [:ochameau]]

We may wait to see how platform bug 855866 goes, as the SDK fix may end up being different.

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 945

This pull request uses toplevel-window-ready instead of domwindowopen.
It depends on bug 855866 as toplevel-window-ready needs to be fired a bit earlier, before the document starts loading, but after private browsing bits are set.

Comment 12

[Alexandre Poirot [:ochameau]]

Comment on attachment 737948 [details]
Pull request 945

See comment 11 for patch description.

Comment 13

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/5dafabfff47118fd2a6475e7758ec2e6f042345b
Bug 855484 - Wait for toplevel-window-ready instead of domwindowopen to avoid issues with private windows.

https://github.com/mozilla/addon-sdk/commit/8d780abb65fc92b6e44c2d7dcede2a0510d7dbdb
Merge pull request #945 from ochameau/bug855484-platform

Bug 855484 - Wait for toplevel-window-ready instead of domwindowopen to avoid issues with private windows. r=@erikvold

Comment 14

[[github robot]]

Commits pushed to integration at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/5dafabfff47118fd2a6475e7758ec2e6f042345b
Bug 855484 - Wait for toplevel-window-ready instead of domwindowopen to avoid issues with private windows.

https://github.com/mozilla/addon-sdk/commit/8d780abb65fc92b6e44c2d7dcede2a0510d7dbdb
Merge pull request #945 from ochameau/bug855484-platform