If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

URL loads overriden by about:privatebrowsing in private browsing windows

NEW
Unassigned

Status

()

Firefox
Private Browsing
5 years ago
4 years ago

People

(Reporter: froydnj, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

5 years ago
jdm says he has seen this occasionally; I see it all the time when testing with the patches from bug 715376.  In particularly, it consistently happens on Try, though not on my Linux development machine.

The failure mode looks like this:

1. A private browsing window is opened.
2. loadURI loads $OTHER_PAGE.
3. $OTHER_PAGE runs, maybe even firing event handlers and such.
4. Something decides that about:privatebrowsing *really* needs to load.
5. about:privatebrowsing loads, which causes test failures and/or hangs.

I'm guessing this sequence is the reason some pb tests don't loadURI the actual test pages until about:privatebrowsing hits pageshown.  But that doesn't seem to matter in a bug 715376 world.

Comment 1

5 years ago
I suspect this is caused by the delayed startup work that happens in browsers: http://hg.mozilla.org/mozilla-central/annotate/c95439870e05/browser/base/content/browser.js#l1331

In particular, if a default argument exists (such as about:privatebrowsing) for a new window, it will merrily go ahead and execute the load.
(Reporter)

Comment 2

5 years ago
(In reply to Josh Matthews [:jdm] from comment #1)
> I suspect this is caused by the delayed startup work that happens in
> browsers:
> http://hg.mozilla.org/mozilla-central/annotate/c95439870e05/browser/base/
> content/browser.js#l1331
> 
> In particular, if a default argument exists (such as about:privatebrowsing)
> for a new window, it will merrily go ahead and execute the load.

Yeah, that seems to be what's happening.  So this code:

http://hg.mozilla.org/mozilla-central/annotate/c95439870e05/browser/base/content/browser.js#l1341

should be changed, I think.  I see that the isLoadingBlank line and the check for about:blank were added from bug 756313 by Dao.  Is the Right Thing to use isLoadingBlank instead of about:blank there (presumably there's a reason Dao didn't use it?), or to check about:privatebrowsing in addition to about:blank?
Flags: needinfo?(dao)
(In reply to Nathan Froyd (:froydnj) from comment #2)
> (In reply to Josh Matthews [:jdm] from comment #1)
> > I suspect this is caused by the delayed startup work that happens in
> > browsers:
> > http://hg.mozilla.org/mozilla-central/annotate/c95439870e05/browser/base/
> > content/browser.js#l1331
> > 
> > In particular, if a default argument exists (such as about:privatebrowsing)
> > for a new window, it will merrily go ahead and execute the load.
> 
> Yeah, that seems to be what's happening.  So this code:
> 
> http://hg.mozilla.org/mozilla-central/annotate/c95439870e05/browser/base/
> content/browser.js#l1341
> 
> should be changed, I think.  I see that the isLoadingBlank line and the
> check for about:blank were added from bug 756313 by Dao.  Is the Right Thing
> to use isLoadingBlank instead of about:blank there (presumably there's a
> reason Dao didn't use it?), or to check about:privatebrowsing in addition to
> about:blank?

If we excluded about:privatebrowsing there, we would fail to load that in a new private window.
Flags: needinfo?(dao)
(Reporter)

Comment 4

5 years ago
(In reply to Dão Gottwald [:dao] from comment #3)
> > Yeah, that seems to be what's happening.  So this code:
> > 
> > http://hg.mozilla.org/mozilla-central/annotate/c95439870e05/browser/base/
> > content/browser.js#l1341
> > 
> > should be changed, I think.  I see that the isLoadingBlank line and the
> > check for about:blank were added from bug 756313 by Dao.  Is the Right Thing
> > to use isLoadingBlank instead of about:blank there (presumably there's a
> > reason Dao didn't use it?), or to check about:privatebrowsing in addition to
> > about:blank?
> 
> If we excluded about:privatebrowsing there, we would fail to load that in a
> new private window.

That doesn't seem to be the case.  Building with checking isLoadingBlank instead of about:blank and then either:

1) Opening a new private window loads; or
2) Starting with -private

loads about:privatebrowsing just fine.  Why shouldn't it work?
Flags: needinfo?(dao)
(In reply to Nathan Froyd (:froydnj) from comment #4)
> > If we excluded about:privatebrowsing there, we would fail to load that in a
> > new private window.
> 
> That doesn't seem to be the case.  Building with checking isLoadingBlank
> instead of about:blank and then either:
> 
> 1) Opening a new private window loads; or
> 2) Starting with -private
> 
> loads about:privatebrowsing just fine.

What code loads about:privatebrowsing then and what's the point of uriToLoad being "about:privatebrowsing" if we don't actually want to load it there?
Flags: needinfo?(dao)

Comment 6

5 years ago
The problem here is that tests will call OpenBrowserWindow({private: true}) and then initiate a load of a content page in the resulting window (usually in the load handler for the chrome window). This then is overwritten by the delayed about:blank load.
Why can't we just wait for about:pb to load first?

Comment 8

5 years ago
That's an option, but does end up moving the burden into every test. It would be nice if we could transparently not load the original URI if a load is already in progress.
(Reporter)

Comment 9

5 years ago
(In reply to :Ehsan Akhgari from comment #7)
> Why can't we just wait for about:pb to load first?

Some private browsing tests do that already:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/mochitest/test_bug_461710_perwindowpb.html#170

(That might be taking it to extremes.)

It would sure be useful if we could deposit that testOnWindow code somewhere where all the private browsing tests could use it.  Is there a good place to put stuff like that?
(In reply to comment #9)
> (In reply to :Ehsan Akhgari from comment #7)
> > Why can't we just wait for about:pb to load first?
> 
> Some private browsing tests do that already:
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/mochitest/test_bug_461710_perwindowpb.html#170
> 
> (That might be taking it to extremes.)
> 
> It would sure be useful if we could deposit that testOnWindow code somewhere
> where all the private browsing tests could use it.  Is there a good place to
> put stuff like that?

Hrm, maybe SimpleTest?  Check with Ted to make sure.
(In reply to Josh Matthews [:jdm] from comment #6)
> The problem here is that tests will call OpenBrowserWindow({private: true})
> and then initiate a load of a content page in the resulting window (usually
> in the load handler for the chrome window). This then is overwritten by the
> delayed about:blank load.

This doesn't answer my question. _delayedStartup currently loads about:privatebrowsing and if you prevent it from doing that via checking isLoadingBlank, I wonder what causes about:privatebrowsing to load instead (comment 4).
(Reporter)

Comment 12

5 years ago
(In reply to Dão Gottwald [:dao] from comment #11)
> (In reply to Josh Matthews [:jdm] from comment #6)
> > The problem here is that tests will call OpenBrowserWindow({private: true})
> > and then initiate a load of a content page in the resulting window (usually
> > in the load handler for the chrome window). This then is overwritten by the
> > delayed about:blank load.
> 
> This doesn't answer my question. _delayedStartup currently loads
> about:privatebrowsing and if you prevent it from doing that via checking
> isLoadingBlank, I wonder what causes about:privatebrowsing to load instead
> (comment 4).

Running |firefox -P empty -private-window -new-instance| inside GDB and breaking inside nsDocShell::LoadURI says:

(gdb) call DumpJSStack()
0 loadURIWithFlags() ["chrome://global/content/bindings/browser.xml":156]
    this = [object XULElement]
1 loadURI() ["chrome://global/content/bindings/browser.xml":127]
    this = [object XULElement]
2 loadURI() ["chrome://browser/content/tabbrowser.xml":2370]
    this = [object XULElement]
3 loadTabs() ["chrome://browser/content/tabbrowser.xml":1168]
    this = [object XULElement]
4 anonymous() ["chrome://browser/content/browser.js":8205]
    this = [object Object]

AFAICS, that is this call:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1354

The codepath for opening a new window is different, it goes through:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1376

It looks like isBlankPageURL isn't returning the right value...I would use the browser debugger to figure out why, but setting breakpoints doesn't appear to be doing the right thing, and I can't dump() from that function.
Blocks: 851006

Comment 13

4 years ago
I guess this is unlikely to be related, but…

[only with my profile for 25.0 Beta 1; disabling extensions helps partly, see below]

In a Private Browsing window, *shortly* after opening a new tab, whatever I put into the address bar is selected, which means I lose the first couple of characters of the text (e.g. the bookmark keyword).

It happens only with my profile.  Disabling extensions helps partly by shortening the time a couple of times.  I have many non-PB tabs in multiple tab groups.
You need to log in before you can comment on or make changes to this bug.