1168768 - Trying to log in with the "Secure Login" addon button crashes the browser

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

STR:
- make sure to have the Secure Login installed ( https://addons.mozilla.org/en-US/firefox/addon/secure-login/ )
- go to a website where login/password information is saved
- try to login with the Secure Login button (window with key at the bottom right of the icon)

Expected:
- login works, as in previous firefox and nightlies (maybe a week or two ago)

Observed:
https://crash-stats.mozilla.com/report/index/3cfb694e-5703-497a-83de-2334f2150526

Comment 1

[:Gijs (he/him)]

The code that causes the tab crash does this (snipped heavily for brevity; note that this runs on the parent process side):

        // The document containing the form:
        var doc = this.getDoc(win);
        // The index for the form containing the login fields:
        var formIndex = this.secureLoginsFormIndex[selectedIndex];
        // The login form:
        var form = doc.forms[formIndex];
        // The form elements list:
        var elements = form.elements;
        // User + Pass fields:
        var usernameField = this.secureLoginsUserField[selectedIndex];
        var passwordField = this.secureLoginsPassField[selectedIndex];
        // Get the target url from the form action value or if empty from the current document:
        var url = form.action ? form.action : doc.baseURI;
        // Reset failed bookmark-login:
        this.failedBookmarkLogin = null;
        // If JavaScript protection is to be used, check the exception list:
        var useJavaScriptProtection = this.secureLoginPrefs.getBoolPref('javascriptProtection');
        if (useJavaScriptProtection && this.inArray(this.getExceptions(), doc.location.protocol + '//' + doc.location.host))
          useJavaScriptProtection = false;
        // Send login data without using the form:
        if (useJavaScriptProtection) {
          // SNIPPED
        } else {
          // Fill the login fields:
          if (usernameField)
            usernameField.value = this.getUsernameFromLoginObject(this.secureLogins[selectedIndex]);
          passwordField.value = this.getPasswordFromLoginObject(this.secureLogins[selectedIndex]);
          if (this.secureLoginPrefs.getBoolPref('autoSubmitForm')) {
            // Prevent multiple submits (e.g. if submit is delayed) by setting a variable (after click on a submit button):
            var submitted = false;
            // Search for the submit button:
            for (var i = 0; i < elements.length; i++) {
              // auto-login by clicking on the submit button:
              if (elements[i].type && elements[i].type == 'submit') {
                elements[i].click();
                submitted = true;
                break;
              }
            }

It finds a submit button (which will be a CPOW, if I understand this correctly - all of these are CPOWs, I would imagine) and calls click() on it, and that immediately crashes the tab. Mike, any idea why?

Comment 2

[Alice0775 White]

bp-1ce50daa-b3c7-4f96-b7f2-fbe192150527

Pushlog:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=70b8d85a0fcc&tochange=c40ca2a73105

Comment 3

[Alice0775 White]

Suspect : bug 1145754

Comment 4

[:Gijs (he/him)]

Matt, thoughts about why this is breaking? :-)

Comment 5

[:Gijs (he/him)]

And I guess I was wrong and this belongs in pwmgr after all...

Comment 6

[Matthew N. [:MattN]]

(In reply to :Gijs Kruitbosch from comment #4)
> Matt, thoughts about why this is breaking? :-)

Yes, if you can look at the stack you can see it's an assertion: https://hg.mozilla.org/mozilla-central/annotate/c6bbf8f1b02b/ipc/glue/MessageChannel.cpp#l799

"can't send sync message of a lesser priority than what's being dispatched"

This is because they're using a CPOW and I added a sendSyncMessage. I think the extension should be updated to not use a CPOW but I don't really understand the assertion so maybe billm knows a solution that doesn't require an extension change.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yes. The add-on needs to be updated to avoid CPOWs.

Comment 8

[Matthew N. [:MattN]]

Needinfo'ing the extension author.

Comment 9

[Matthew N. [:MattN]]

The other author can be contacted using the Contact link at http://firefox.add0n.com/secure-login.html

Comment 10

[Sebastian Tschan]

Original Secure Login author here.

Unfortunately I've not done any Firefox Add-On development for years and don't maintain those projects anymore.

The current maintainer is Sarah Avilov, but not sure if she's still active:
https://addons.mozilla.org/en-US/firefox/user/SarahAvilov/

Comment 11

[Jorge Villalobos [:jorgev] (he/him)]

At least the user account has recent activity. I sent a message to the developer through AMO.

Comment 12

[Sarah Avilov]

Sarah (the new developer) is here

I pushed a new beta version that fixes the crash
https://addons.mozilla.org/firefox/downloads/file/318888/secure_login-1.1.0b2-fx.xpi?src=devhub

In case it matters, whenever the extension tries to execute `.click()`, `.submit()` or `.focus()` on a DOM element from the chrome context, e10s crashes. To prevent the crash I am using messageManager to perform these actions.

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Sarah Avilov from comment #12)
> Sarah (the new developer) is here
> 
> I pushed a new beta version that fixes the crash
> https://addons.mozilla.org/firefox/downloads/file/318888/secure_login-1.1.
> 0b2-fx.xpi?src=devhub
> 
> In case it matters, whenever the extension tries to execute `.click()`,
> `.submit()` or `.focus()` on a DOM element from the chrome context, e10s
> crashes. To prevent the crash I am using messageManager to perform these
> actions.

For what it's worth, the new version of the addon works locally and I am not seeing this crash anymore. Thanks for the update!