847991 - URL loads overriden by about:privatebrowsing in private browsing windows

Status: NEW

(Firefox :: Private Browsing, defect)

Description

[Nathan Froyd [:froydnj]]

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

[Josh Matthews [:jdm]]

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.

Comment 2

[Nathan Froyd [:froydnj]]

(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?

Comment 3

[Dão Gottwald [: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.

Comment 4

[Nathan Froyd [:froydnj]]

(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?

Comment 5

[Dão Gottwald [: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?

Comment 6

[Josh Matthews [:jdm]]

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.

Comment 7

[(no longer active)]

Why can't we just wait for about:pb to load first?

Comment 8

[Josh Matthews [:jdm]]

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.

Comment 9

[Nathan Froyd [:froydnj]]

(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?

Comment 10

[(no longer active)]

(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.

Comment 11

[Dão Gottwald [:dao]]

(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).

Comment 12

[Nathan Froyd [:froydnj]]

(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.

Comment 13

[[:Aleksej]]

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.

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 14 votes.
:timhuang, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 15

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.