1078085 - Firefox 35.0a1 (2014-10-05) CTRL+T does not open new tab

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect)

Description

[lussnig]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20141005030205

Steps to reproduce:

Hit the combination CTRL+T


Actual results:

Nothing happens-


Expected results:

I excpect an new tab to be opened. Also this does also not work if all adons are disabled.

Comment 1

[Matthias Versen [:Matti]]

works for me with a local buildm revision 0ed32d9a42d6 on windows7

Comment 2

[lussnig]

In the Script Console in found this error:

(browser/omni.ja!/modules/BrowserNewTabPreloader.jsm)

TypeError: this._branch is null BrowserNewTabPreloader.jsm:90

Comment 3

[:Gijs (he/him)]

Do you know when this last worked?

Comment 4

[lussnig]

It worked on Friday 2014-10-02. Is there any other information that i can provide ?

Comment 5

[:Gijs (he/him)]

(In reply to lussnig from comment #4)
> It worked on Friday 2014-10-02. Is there any other information that i can
> provide ?

Do you use a custom new tab page? Can you provide the contents of about:support (help > troubleshooting information) ? Do you have e10s turned on?


Tim, any ideas here?

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

I'll just assume this was my fault with bug 1073993.

Comment 7

[lussnig]

Attachment: Support.txt

Raw-Content of about:support is in Support.txt
E10S is disabled.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1078085-Simplify-preference-checking-code-in-the.patch

Good opportunity to just get rid of all the stupid prefs code in BrowserNewTabPreloader.jsm alltogether. Had to fix about:newtab's intro.js because that was testing whether the intro would be shown for a preloaded newtab page as well - turns out it didn't properly test that.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8501155 [details] [diff] [review]
0001-Bug-1078085-Simplify-preference-checking-code-in-the.patch

Review of attachment 8501155 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/BrowserNewTabPreloader.jsm
@@ +37,5 @@
>  const BROWSER_CONTENT_SCRIPT = "chrome://browser/content/content.js";
>  
> +function isPreloadingEnabled() {
> +  return Services.prefs.getBoolPref(PREF_NEWTAB_PRELOAD);
> +         !Services.prefs.prefHasUserValue(PREF_NEWTAB_URL);

Oops. I'll fix that.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1078085-Simplify-preference-checking-code-in-the.patch, v2

Comment 11

[lussnig]

After start with all plugins disabled i created an new profile, that worked.
Then i used "prefs.js" from the other profile it did not work.
I than could limit the cause to one line in prefs.js

user_pref("dom.indexedDB.enabled", false);

This line disable the New Tab hotkey.
Is there any reason that indexedDB is related to hotkey functions ?

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

(In reply to lussnig from comment #11)
> Is there any reason that indexedDB is related to hotkey functions ?

No, I don't think that's what causes your issue but it might be some weird correlation. We should have a proper fix for this soon. Thanks for investigating further!

Comment 13

[lussnig]

I checked it again. Toggle this flag directly have two effects:
- disable new tab hotkey
- disable save new passwords for pages

Reproduce:
1) Goto about:config 
2) Search dom.indexedDB.enabled
3) Toggle the value
4) Restart Firefox

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

(In reply to lussnig from comment #13)
> I checked it again. Toggle this flag directly have two effects:
> - disable new tab hotkey
> - disable save new passwords for pages

Yeah, I would suggest not toggling this preference. A lot of internal features rely on IndexedDB and that's why we do not expose that in the UI as it can break stuff.

Comment 15

[:Gijs (he/him)]

(In reply to Tim Taubert [:ttaubert] from comment #14)
> (In reply to lussnig from comment #13)
> > I checked it again. Toggle this flag directly have two effects:
> > - disable new tab hotkey
> > - disable save new passwords for pages
> 
> Yeah, I would suggest not toggling this preference. A lot of internal
> features rely on IndexedDB and that's why we do not expose that in the UI as
> it can break stuff.

Filed bug 1079355 to get rid of this pref.

Comment 16

[:Gijs (he/him)]

Comment on attachment 8501160 [details] [diff] [review]
0001-Bug-1078085-Simplify-preference-checking-code-in-the.patch, v2

Review of attachment 8501160 [details] [diff] [review]:
-----------------------------------------------------------------

Err, this is a big change. Why don't we just make Preferences.enabled call Preferences.init if _branch is null? That'd be a much smaller fix and would work just as well, AFAICT, and doesn't cause us to read prefs every time we open a new tab...

::: browser/base/content/newtab/intro.js
@@ -6,5 @@
>  
>  const PREF_INTRO_SHOWN = "browser.newtabpage.introShown";
>  
>  let gIntro = {
> -  _introShown: Services.prefs.getBoolPref(PREF_INTRO_SHOWN),

Can we cache this pref value somehow or does that not make sense?

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gijs Kruitbosch from comment #16)
> Err, this is a big change.

Aww, c'mon it's not that big :)

> Why don't we just make Preferences.enabled call
> Preferences.init if _branch is null? That'd be a much smaller fix and would
> work just as well, AFAICT, and doesn't cause us to read prefs every time we
> open a new tab...

Reading prefs is cheap. I think we should stop adding caches and observers everywhere just to avoid reading prefs twice. There is a lot going on when opening tabs and reading a pref or two is really negligible overhead.

> > -  _introShown: Services.prefs.getBoolPref(PREF_INTRO_SHOWN),
> 
> Can we cache this pref value somehow or does that not make sense?

It's only called once so probably not.

Comment 18

[lussnig]

> Yeah, I would suggest not toggling this preference. A lot of internal
> features rely on IndexedDB and that's why we do not expose that in the UI as
> it can break stuff.
Maybe i am an bit naive. But i think that there should be an cut between internal code.
Like an 3 Level Security System:
- Internal Stuff have unrestricted access to api's
- Plugin Code 
- Javascript Code on WebPages.

For example i have no problem it Firefox use indexedDB for internal properties.
But i have real problems if any webpage can store data in an indexedDB on local pc.

Comment 19

[:Gijs (he/him)]

Comment on attachment 8501160 [details] [diff] [review]
0001-Bug-1078085-Simplify-preference-checking-code-in-the.patch, v2

Review of attachment 8501160 [details] [diff] [review]:
-----------------------------------------------------------------

Alright, let's see what this does to perf. Ship it.

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fa2f6c1298bf

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/2c435535573d

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2c435535573d