Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#11783 closed defect (fixed)

Torbutton: Unexpected error on new identity: TypeError: b.webProgress is undefined

Reported by: cypherpunks Owned by: mikeperry
Priority: Medium Milestone:
Component: TorBrowserButton Version:
Severity: Keywords: tbb-newnym, MikePerry201406R
Cc: gk Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Anybody yet observes this alert?
TBB-3.6
Everything worked fine before, and then suddenly alert message box. Last automatically updated add-on was Noscript 2.6.8.23

Debugging showing that exception generated by

browser.addTab("about:blank");

from torbutton_close_on_toggle()

Can't reliably reproduce this bug, everything works and then for the same sequence of actions this fail appears.

Child Tickets

Change History (44)

comment:1 Changed 4 years ago by cypherpunks

Noscript 2.6.8.23

({73a6fe31-595d-460b-a920-fcc0f8843232}.xpi)= 0c8049335f2c2630fc5dd0d37dcf99e0dbe5b0e47b2930acf88d56a6fa52ffbc

comment:2 Changed 4 years ago by gk

Cc: gk added

comment:3 Changed 4 years ago by cypherpunks

Last edited 4 years ago by cypherpunks (previous) (diff)

comment:4 Changed 4 years ago by cypherpunks

Last edited 4 years ago by cypherpunks (previous) (diff)

comment:5 Changed 4 years ago by cypherpunks

Last edited 4 years ago by cypherpunks (previous) (diff)

comment:6 Changed 4 years ago by cypherpunks

Last edited 4 years ago by cypherpunks (previous) (diff)

comment:7 Changed 4 years ago by cypherpunks

Last edited 4 years ago by cypherpunks (previous) (diff)

comment:8 Changed 4 years ago by cypherpunks

Output to console

Torbutton WARN: DocShell is null for unparsable URL: TypeError: this.docShell is null
Torbutton WARN: Unexpected error on new identity: TypeError: b.webProgress is undefined
this.docShell is null @ chrome://global/content/bindings/browser.xml:323
Last edited 4 years ago by cypherpunks (previous) (diff)

comment:9 Changed 4 years ago by cypherpunks

Alert happens with freshly installed TBB-3.6 with disabled add-on updates. Didn't happen for week before and now it alerts and alerts for the same code. Suspicious.

If nobody else observes that alert then need to close bug with "works for me" resolution.
"b.webProgress is undefined" appears in mozilla's bugzilla and every ticket was closed with that resolution, some race condition that unrepairable, broken by design.

comment:10 Changed 4 years ago by cypherpunks

Resolution: worksforme
Status: newclosed

comment:11 Changed 4 years ago by cypherpunks

Last edited 4 years ago by cypherpunks (previous) (diff)

comment:12 Changed 4 years ago by gk

Resolution: worksforme
Status: closedreopened

comment:13 Changed 4 years ago by cypherpunks

Last edited 4 years ago by cypherpunks (previous) (diff)

comment:14 Changed 4 years ago by cypherpunks

Last edited 4 years ago by cypherpunks (previous) (diff)

comment:15 Changed 4 years ago by cypherpunks

Last edited 4 years ago by cypherpunks (previous) (diff)

comment:16 Changed 4 years ago by cypherpunks

Last edited 4 years ago by cypherpunks (previous) (diff)

comment:17 Changed 4 years ago by cypherpunks

Resolution: wontfix
Status: reopenedclosed

comment:18 Changed 4 years ago by gk

Resolution: wontfix
Status: closedreopened

comment:19 Changed 4 years ago by gk

Keywords: tbb-newnym added

comment:20 Changed 4 years ago by gk

Hey, cypherpunk: good news, I experienced the same problem. And even better, there is some weird stuff in the error console

[18:26:13.985] [05-10 18:26:13] Torbutton WARN: DocShell is null for unparsable URL: TypeError: this.docShell is null
[18:26:14.068] [05-10 18:26:14] Torbutton WARN: Unexpected error on new identity: TypeError: b.webProgress is undefined
[18:26:14.113] [05-10 18:26:14] Torbutton WARN: DocShell is null for unparsable URL: TypeError: this.docShell is null
[18:26:14.113] [05-10 18:26:14] Torbutton WARN: DocShell is null for unknown URL
[18:26:14.181] [05-10 18:26:14] Torbutton WARN: Unexpected error on new identity: TypeError: b.webProgress is undefined
[18:26:14.397] this.docShell is null @ chrome://global/content/bindings/browser.xml:323

And, yes, leave the ticket open.

comment:21 Changed 4 years ago by cypherpunks

Interesting. Looks like at least two call of torbutton_new_identity in a row without any pause.

[18:26:14.068]
[18:26:14.181]

comment:22 Changed 4 years ago by gk

Another interesting thing: if this error happens and I press "New Identity" afterwards again my Tor Browser gets shut down.

comment:23 Changed 4 years ago by gk

My mouse was broken which made me hit this, I guess. If I double-click on New Identity fast I can reproduce this issue quite reliably.

comment:24 Changed 4 years ago by cypherpunks

If I double-click on New Identity fast I can reproduce this issue quite reliably.

Can't reproduce this with auto-click generator. Can to open 2-5 tabs by rapid clicks, but menu items called only once even if 1ms interval.

comment:25 Changed 4 years ago by cypherpunks

Maybe depends double click time-out preferences used by environment. If double click was detected then bug reproducible but still not every time.

comment:26 Changed 4 years ago by cypherpunks

No, it's not about double click timeout, disabled double click detection and still caught alert on fast rapid clicks.

comment:27 Changed 4 years ago by cypherpunks

And double click is not enough to reproduce bug. This bug appears randomly for some hours (days) and then vanishes. Moon phases, weather, NSA?

comment:28 Changed 4 years ago by cypherpunks_backup

If this bug still reproducible for you, can you test this:

--- popup.xul
+++ popup.xul.modified
@@ -14,6 +14,7 @@
         <menuitem id="torbutton-new-identity"
                   label="&torbutton.context_menu.new_identity;"
                   accesskey="&torbutton.context_menu.new_identity_key;"
+                  disabled="true"
                   insertafter="context-stop"
                   oncommand="torbutton_new_identity()"/>
         <menuitem  id="torbutton-cookie-protector"

--- torbutton.js
+++ torbutton.js.modified
@@ -1430,6 +1430,7 @@
 
 // Bug 1506 P4: Needed for New Identity.
 function torbutton_new_identity() {
+  document.getElementById("torbutton-new-identity").disabled = true;
   try {
     torbutton_do_new_identity();
   } catch(e) {
@@ -2105,6 +2106,8 @@
 
   if (!m_tb_control_pass || !m_tb_control_port)
     document.getElementById("torbutton-new-identity").disabled = true;
+  else
+    document.getElementById("torbutton-new-identity").disabled = false;
 
   if (!m_tb_tbb && m_tb_prefs.getBoolPref("extensions.torbutton.prompt_torbrowser")) {
       torbutton_inform_about_tbb();

Can it to prevent bug?

comment:29 Changed 4 years ago by cypherpunks

Or even simpler:

--- torbutton.js
+++ torbutton.js.modified
@@ -1430,6 +1430,7 @@
 
 // Bug 1506 P4: Needed for New Identity.
 function torbutton_new_identity() {
+  document.getElementById("torbutton-new-identity").disabled = true;
   try {
     torbutton_do_new_identity();
   } catch(e) {

This should be the same.
But why torbutton_new_identity called twice at all? without answer even this kludge can't guarantee such protection actually works for every case.

comment:30 in reply to:  29 Changed 4 years ago by gk

Keywords: MikePerry201405 added
Status: reopenedneeds_review

Replying to cypherpunks:

But why torbutton_new_identity called twice at all? without answer even this kludge can't guarantee such protection actually works for every case.

Not sure what you mean, but clicking on the menuitem n times is executing the function, which is bound to it, n times (approximately; it depends on how fast you and probably your computer are). Anyway, see bug_11783 in my torbutton repo for a fix.

Last edited 4 years ago by gk (previous) (diff)

comment:31 Changed 4 years ago by cypherpunks

but clicking on the menuitem n times is executing the function, which is bound to it, n times (approximately; it depends on how fast you and probably your computer are)

You can't to click twice on the same menu item, oncommand should be called only once or menu should never be destroyed. For menu popups only 1 click is allowed, all next is state of art.

document.getElementById("torbutton-new-identity").disabled = false;

Why to re-enable it on exception? What purpose? Those element doesn't exist after oncommand finished. You can't to keep menu on exception too. Element recreated on every new menu popup with default state.

comment:32 in reply to:  31 Changed 4 years ago by gk

Replying to cypherpunks:

but clicking on the menuitem n times is executing the function, which is bound to it, n times (approximately; it depends on how fast you and probably your computer are)

You can't to click twice on the same menu item, oncommand should be called only once or menu should never be destroyed. For menu popups only 1 click is allowed, all next is state of art.

document.getElementById("torbutton-new-identity").disabled = false;

Why to re-enable it on exception? What purpose? Those element doesn't exist after oncommand finished. You can't to keep menu on exception too. Element recreated on every new menu popup with default state.

Ah, right. That's one episode in the "Think Once, Commit Twice" saga... bug_11783_v2 in my repo is the new hotness.

comment:33 in reply to:  31 Changed 4 years ago by gk

Replying to cypherpunks:

but clicking on the menuitem n times is executing the function, which is bound to it, n times (approximately; it depends on how fast you and probably your computer are)

You can't to click twice on the same menu item, oncommand should be called only once or menu should never be destroyed. For menu popups only 1 click is allowed, all next is state of art.

You might be right here. However, this is not the whole story. I see in fact the second execution in my logs although only after we called OpenBrowserWindow(). My guess is that the second execution happens somehow in this new window context which is not properly initialized yet. Anyway, I don't have the time to look deeper at it right now to understand what is exactly going on under the hood.

comment:34 Changed 4 years ago by cypherpunks

My guess is that the second execution happens somehow in this new window context which is not properly initialized yet.

I think second execution happens for the same menu popup as for first click. Browser keeps menu while context was destroyed or should be destroyed, race condition is about order of destroy for menu popup vs tabs. Or no races even, browser destroys context of window first while menu exist till the end. Bug is about speedness of user vs. speed of code execution (possible depends jit?).

comment:35 Changed 4 years ago by gk

Keywords: MikePerry201405R added; MikePerry201405 removed

comment:36 Changed 4 years ago by mikeperry

I think we want to take the version that re-enables the new identity button if there was an exception. That menu will not be redrawn each time it is opened, as it is sourced from the torbutton.xul overlay, which is bound to browser.xul. It will only be re-created if the browser window is successfully closed and a new one is opened.

This makes me think that gk/bug_11783 is the right branch to merge here, so that exceptions don't completely disable New Identity.

comment:37 Changed 4 years ago by cypherpunks

I think we want to take the version that re-enables the new identity button if there was an exception. That menu will not be redrawn each time it is opened, as it is sourced from the torbutton.xul overlay, which is bound to browser.xul. It will only be re-created if the browser window is successfully closed and a new one is opened.

This theory easy to test, no need to guess.

comment:38 Changed 4 years ago by cypherpunks

This makes me think that gk/bug_11783 is the right branch to merge here, so that exceptions don't completely disable New Identity.

You're right. Just tested, it need to re-enable menu item if caught exception.

comment:39 Changed 4 years ago by cypherpunks

it need to re-enable menu item if caught exception.

But as shown this bug, every next click in the same context could leads to crash/exit.

comment:40 in reply to:  39 Changed 4 years ago by mikeperry

Keywords: MikePerry201406R added; MikePerry201405R removed

Replying to cypherpunks:

it need to re-enable menu item if caught exception.

But as shown this bug, every next click in the same context could leads to crash/exit.

Hrmm.. I wonder why the crash/exit is happening.. The undefined error should not cause that.. Leaving New Identity greyed seems to just hide that issue?

Should we file a different ticket for the crash? Should I revert this merge?

comment:41 Changed 4 years ago by mikeperry

Resolution: fixed
Status: needs_reviewclosed

Ok, so on IRC gk and I agreed to file a different ticket for the unexpected exit issue.

comment:42 Changed 4 years ago by cypherpunks

Leaving New Identity greyed seems to just hide that issue?

Leaving New Identity greyed hides bug in design of browser's interface. In first it's bug to process several clicks for menu items in the same context. All another races and crashes just a result of this bug.

comment:43 in reply to:  41 Changed 4 years ago by gk

Replying to mikeperry:

Ok, so on IRC gk and I agreed to file a different ticket for the unexpected exit issue.

On a second thought comment 39 is wrong and we don't need a new ticket. The "click-and-the browser-crashes"-issue existed only due to the bug we fixed here. So, re-enabling the "New Identity"-option is fine and won't lead to a browser shutdown anymore. Everything is good.

comment:44 Changed 4 years ago by cypherpunks

Everything is good.

Aha. Except browser's brokenness.

Note: See TracTickets for help on using tickets.