Closed Bug 793715 Opened 8 years ago Closed 8 years ago

Switch to Tab no more 0.1 breaks awesomebar since 20120923 Nightly

Categories

(Toolkit :: XUL Widgets, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla18
Tracking Status
firefox18 + verified

People

(Reporter: ehoogeveen, Assigned: jaws)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

Since the 20120923 Nightly my awesomebar has been broken. Thanks to some digging by Pr0phet[1] we found that "Switch to Tab no more 0.1"[2] was responsible for the issues. This might be INVALID if the this is an intentional change, but I figured I'd file it anyway. Looking at the changelog for yesterday's Nightly, I think bug 781588 might be responsible.

Expected results:
Awesomebar continues to work as expected, with the "Switch to tab" option removed from the dropdown history.

Actual results:
Dropdown history does not show up until the arrow keys are pressed several times, and does not *update* until text is cleared and typing begins anew. In addition, pasting text into the awesomebar is broken.

[1] http://forums.mozillazine.org/viewtopic.php?p=12317369#p12317369
[2] https://addons.mozilla.org/en-US/firefox/addon/switch-to-tab-no-more/
Emanuel,

Maybe ask at http://forums.mozillazine.org/viewtopic.php?f=38&t=2287733 which is listed as "Support site" for "Switch to Tab no more".
I just installed "Switch to Tab no more" on the 9-24 Nightly and I'm not seeing any issues with the Awesome Bar.

Can you include some better steps to reproduce and reopen?
Status: UNCONFIRMED → RESOLVED
Closed: 8 years ago
Resolution: --- → INVALID
Interesting, I can confirm that the problem happens with all other extensions disabled, so there might be some other interaction with my profile. Will do more testing in the morning.
Okay, for me the following steps consistently reproduce the behavior I reported above:

1) From a clean profile, navigate to https://addons.mozilla.org/en-US/firefox/addon/switch-to-tab-no-more/ and install the extension.
2) Restart the browser to finish the installation. At this point, pasting things into the address bar should be broken.
3) Navigate to http://twitch.tv/team/srl and restart the browser.
4) Type the letter 's' into the address bar. No history dropdown should appear until the arrow keys are used (and what does appear isn't dynamic, i.e. it doesn't depend on what you type afterward until you clear the text and start over with the dropdown still open).

Hopefully that will give you enough info to reproduce.
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---
Can you disable browser.urlbar.formatting.enabled in about:config and try again? The code that I changed will be disabled if that pref is disabled.
browser.urlbar.formatting.enabled
https://addons.mozilla.org/en-US/firefox/addon/switch-to-tab-no-more/

Apologies for the delayed response, but I found something curious in testing and needed a bit more free time to confirm. As it turns out, using the steps I listed in comment #4, the behavior I mentioned disappears after restarting the browser once more. Pasting works again, and so does the history dropdown. As this is hardly the case for my regular profile, it seems I don't know how to get it into the broken state permanently.

However, changing the setting you mentioned *prior* to installing the extension, I did notice a change in behavior:
1) From a clean profile, go to about:config and set browser.urlbar.formatting.enabled to false, then restart the browser.
2) Navigate to https://addons.mozilla.org/en-US/firefox/addon/switch-to-tab-no-more/ and install the extension.
3) Restart the browser to finish the installation. At this point, pasting things into the address bar should be broken.
4) Navigate to http://twitch.tv/team/srl and restart the browser.
5) Type the letter 's' into the address bar. Unlike before, the history dropdown should appear as normal. However, pasting things into the address bar is still broken.

Restarting the browser again after this point will restore both the history dropdown and pasting behavior to normal regardless of browser.urlbar.formatting.enabled. So the behavior is a bit.. elusive, at least with the STR I used here. Hopefully the difference I found will still give you a clue as to what could be going on.
Ah, those two lines at the top of my comment were left over from my notepad as I was testing and writing the comment.
I had the same issue too, though I have no reliable repro steps. It seems that a well populated Places database is required for this bug to manifest.

The browser.urlbar.formatting.enabled pref doesn't seem to have any effect.
Well, I was able to confirm that bug 781588 caused this regression:

Last good (local build): 
https://hg.mozilla.org/integration/mozilla-inbound/rev/9108fa673bfe

First bad (local build):
https://hg.mozilla.org/integration/mozilla-inbound/rev/f753afbe0d85

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9108fa673bfe&tochange=f753afbe0d85

If my STR aren't enough to allow you to debug this, could I e-mail you the relevant parts of my profile?
Do you see any errors in your Error Console? And if you set browser.urlbar.formatting.enabled to false then the problem goes away?
Negative to both questions.
browser.urlbar.formatting.enabled=false completely disables the code changed by bug 781588. Can you try with a new profile?
Reproducing the issue in a new profile with a manually copied, populated places database is rare (though it does happen some times) and I couldn't correlate the pref state with the issue happening :( .

The good news it that using my normal profile, where the issue is reliably reproducible, the state of the pref *does* make a difference in the bug appearing this time. I don't remember if I restarted the browser last time I tried the pref so sorry for any confusion.

Will give it another try tomorrow
The following entries spam the Error Console.

Timestamp: 07/10/2012 01:01:31
Error: TypeError: this.editor is null
Source File: chrome://browser/content/urlbarBindings.xml
Line: 159

Timestamp: 07/10/2012 01:03:54
Error: TypeError: this.editor is null
Source File: chrome://browser/content/urlbarBindings.xml
Line: 216
I ran some tests with debugging builds of the above changesets and logged the stderr output to text files. In all cases, there were no differences when comparing
1) [lastgood without addon] vs [lastgood with addon]
2) [lastgood without addon] vs [firstbad without addon]

In all cases, there were differences when comparing [firstbad without addon] vs [firstbad with addon]. I didn't test with browser.urlbar.formatting.enabled set to false, let me know if I should.

----------------------

1st case: 
1) Restore session with only about:addons open
2) Wait 30 seconds
3) Exit

Error lines in [no addon] but not in [with addon]:
WARNING: NS_ENSURE_TRUE(mMutable) failed: file c:/Users/firefox/netwerk/base/src/../../../../netwerk/base/src/nsSimpleURI.cpp, line 273
WARNING: NS_ENSURE_TRUE(mMutable) failed: file c:/Users/firefox/netwerk/base/src/../../../../netwerk/base/src/nsSimpleURI.cpp, line 273

Error lines in [with addon] but not in [no addon]:

----------------------

2nd case:
1) Restore session with only about:home open and the text 'srl' in the address bar
2) Wait 30 seconds
3) Select text in address bar, type in 'srl' (so history dropdown appears)
4) Wait 30 seconds
5) Exit

Error lines in [no addon] but not in [with addon]:
WARNING: NS_ENSURE_TRUE(mMutable) failed: file c:/Users/firefox/netwerk/base/src/../../../../netwerk/base/src/nsSimpleURI.cpp, line 273
WARNING: NS_ENSURE_TRUE(mMutable) failed: file c:/Users/firefox/netwerk/base/src/../../../../netwerk/base/src/nsSimpleURI.cpp, line 273
WARNING: NS_ENSURE_TRUE(mMutable) failed: file c:/Users/firefox/netwerk/base/src/../../../../netwerk/base/src/nsSimpleURI.cpp, line 273
WARNING: Nv3DVStreaming CoCreateInstance failed (disabled).: file c:/Users/firefox/gfx/layers/../../../gfx/layers/d3d9/Nv3DVUtils.cpp, line 53

Error lines in [with addon] but not in [no addon]:
JavaScript error: chrome://browser/content/urlbarBindings.xml, line 216: this.editor is null
JavaScript error: chrome://browser/content/urlbarBindings.xml, line 159: this.editor is null

----------------------

3rd case:
1) Restore session with only about:home open no text in the address bar
2) Wait 30 seconds
3) Paste the text 'paste' into the address bar
4) Wait 30 seconds
5) Exit

Error lines in [no addon] but not in [with addon]:
WARNING: NS_ENSURE_TRUE(mMutable) failed: file c:/Users/firefox/netwerk/base/src/../../../../netwerk/base/src/nsSimpleURI.cpp, line 273
WARNING: NS_ENSURE_TRUE(mMutable) failed: file c:/Users/firefox/netwerk/base/src/../../../../netwerk/base/src/nsSimpleURI.cpp, line 273
WARNING: NS_ENSURE_TRUE(mMutable) failed: file c:/Users/firefox/netwerk/base/src/../../../../netwerk/base/src/nsSimpleURI.cpp, line 273
WARNING: Nv3DVStreaming CoCreateInstance failed (disabled).: file c:/Users/firefox/gfx/layers/../../../gfx/layers/d3d9/Nv3DVUtils.cpp, line 53

Error lines in [with addon] but not in [no addon]:
JavaScript error: chrome://browser/content/urlbarBindings.xml, line 216: this.editor is null
JavaScript error: chrome://global/content/bindings/textbox.xml, line 156: this.editor is null
JavaScript error: chrome://global/content/bindings/autocomplete.xml, line 542: this._autocomplete.editor is null
JavaScript error: chrome://browser/content/urlbarBindings.xml, line 159: this.editor is null
Blocks: 781588
Attached patch Patch (obsolete) — Splinter Review
It appears that the changing of textbox.editor to a field is causing issues for some users as the value is being cached before it is useful (hence being null for the full session).

Ehsan, do you know any ways to guarantee that editor is non-null while still using a field? The perf hit from using a property was quite noticeable, but correctness is obviously more important.
Assignee: nobody → jaws
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #668942 - Flags: review?(ehsan)
So this is the overlay that this add-on loads:

http://pastebin.mozilla.org/1860262

I don't see anything there which would cause this kind of problem.  Jared, have you tried setting a breakpoint in nsHTMLInputElement::GetEditor and see why it's returning null?  This should not really happen, and this patch may just wallpaper over a real bug.

Also, why does textbox.xml implement the inputField member as a property on top of a field?  <http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/textbox.xml#51>  I assume there's a good reason for that.  Perhaps that's what needs to be done here?
Component: Extension Compatibility → XUL Widgets
Product: Firefox → Toolkit
Attached patch Patch v2 (obsolete) — Splinter Review
It's returning null because it stops at this point in nsTextEditorState::PrepareEditor,
> if (!mBoundFrame) {
>   // Cannot create an editor without a bound frame.
>   // Don't return a failure code, because js callers can't handle that.
>   return NS_OK;
> }

mBoundFrame is null when this is called. This is the call stack, http://pastebin.mozilla.org/1860315

I looked in the CVS history for the reasoning behind the property-on-top-of-field for inputField, but all I ended up with was https://github.com/jrmuizel/mozilla-cvs-history/commit/95c14745fea088c5b53165901de3acca2a00f6d5.

I switched the patch to use this solution since it will have better perf once this.editor is non-null. This is the callstack when the mBoundFrame is non-null: http://pastebin.mozilla.org/1860321.
Attachment #668942 - Attachment is obsolete: true
Attachment #668942 - Flags: review?(ehsan)
Attachment #668955 - Flags: review?(ehsan)
Comment on attachment 668955 [details] [diff] [review]
Patch v2

Ah, ok, that makes sense.  We can't create an editor before a frame has been constructed for the input element.  This will correctly handle that case.
Attachment #668955 - Flags: review?(ehsan) → review+
Keywords: regression
(This can break more than that add-on.)
Comment on attachment 668956 [details] [diff] [review]
Patch for checkin (added r=ehsan)

>       <field name="mInputField">null</field>
>       <field name="mIgnoreClick">false</field>
>       <field name="mIgnoreFocus">false</field>
>+      <field name="mEditor">false</field>
[I would have used null for consistency with mInputField which is an object rather than a boolean.]
https://hg.mozilla.org/mozilla-central/rev/bf0aa600323f
https://hg.mozilla.org/mozilla-central/rev/2ec21afa445b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
In today's nightly all the broken behaviors are gone. I also repeated the test from comment #15 with local debugging builds using the changesets just before and after the fix, and the errors are all gone - the log files with and without the addon once again match up. Thanks for the fix!
Status: RESOLVED → VERIFIED
Please nominate for uplift to Aurora 18 as soon as possible. Thanks!
No need to track this, it got fixed before the merge so it's already in Aurora 18 :)
(In reply to Jared Wein [:jaws] from comment #28)
> No need to track this, it got fixed before the merge so it's already in
> Aurora 18 :)

Nope, we still need to track it to make sure that it will reappear on radar if the patch gets backed out from 18 or something.  :-)
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:18.0) Gecko/18.0 Firefox/18.0 beta 6
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20130102 Firefox/20.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:18.0) Gecko/18.0 Firefox/18.0 beta 6
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:20.0) Gecko/20130103 Firefox/20.0
Mozilla/5.0 (X11; Linux i686; rv:18.0) Gecko/18.0 Firefox/18.0 beta 6
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20130103 Firefox/20.0
`
Verified as fixed on latest nightly and beta on above builds.
You need to log in before you can comment on or make changes to this bug.