Closed Bug 1168768 Opened 9 years ago Closed 9 years ago

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

Categories

(WebExtensions :: General, defect)

defect
Not set
normal

Tracking

(e10sm8+)

RESOLVED FIXED
Tracking Status
e10s m8+ ---

People

(Reporter: bbouvier, Unassigned)

References

Details

(Keywords: crash, regression)

Crash Data

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
Summary: Trying to log in with the "Secure Login" addon crashes the browser → Trying to log in with the "Secure Login" addon button crashes the browser
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?
Blocks: e10s-addons
Component: Password Manager → Untriaged
Flags: needinfo?(mconley)
Product: Toolkit → Core
tracking-e10s: --- → ?
Suspect : bug 1145754
Blocks: 1145754
Matt, thoughts about why this is breaking? :-)
Flags: needinfo?(mconley) → needinfo?(MattN+bmo)
And I guess I was wrong and this belongs in pwmgr after all...
Component: Untriaged → Password Manager
Product: Core → Toolkit
(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.
Crash Signature: [@ mozalloc_abort(char const*) | NS_DebugBreak | mozilla::ipc::MessageChannel::DebugAbort(char const*, int, char const*, char const*, bool) const | mozilla::ipc::MessageChannel::Send(IPC::Message*, IPC::Message*)]
Flags: needinfo?(MattN+bmo) → needinfo?(wmccloskey)
Keywords: crash
Yes. The add-on needs to be updated to avoid CPOWs.
Flags: needinfo?(wmccloskey)
Component: Password Manager → Add-ons
Product: Toolkit → Tech Evangelism
Needinfo'ing the extension author.
Flags: needinfo?(sebastian.tschan)
The other author can be contacted using the Contact link at http://firefox.add0n.com/secure-login.html
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/
Flags: needinfo?(sebastian.tschan)
At least the user account has recent activity. I sent a message to the developer through AMO.
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.
(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!
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Component: Add-ons → General
Product: Tech Evangelism → WebExtensions
You need to log in before you can comment on or make changes to this bug.