1158496 - JavaScript error: .../nsSuiteGlue.js, line 291: NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIWebProgress.DOMWindow]

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Martin Mokrejs]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:36.0) Gecko/20100101 Firefox/36.0 SeaMonkey/2.33.1
Build ID: 20150425013755

Steps to reproduce:

Not sure if this was triggered by the bugzilla.mozilla.org login process or due to the about:plugins page to be loaded.


Actual results:

--DOMWINDOW == 39 (0x7f25a609c500) [pid = 20201] [serial = 60] [outer = (nil)] [url = https://login.persona.org/communication_iframe]
[20201] WARNING: NS_ENSURE_TRUE(txToRemove) failed: file /var/tmp/portage/www-client/seamonkey-2.33.1/work/comm-release/mozilla/docshell/shistory/src/nsSHistory.cpp, line 1339
JavaScript error: jar:file:///usr/lib64/seamonkey/omni.ja!/components/nsSuiteGlue.js, line 291: NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIWebProgress.DOMWindow]
Document about:plugins loaded successfully

Comment 1

[Philip Chee]

> JavaScript error: jar:file:///usr/lib64/seamonkey/omni.ja!/components/nsSuiteGlue.js,
> line 291: NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIWebProgress.DOMWindow]

I've seen this error too. CC Neil

Comment 2

[[e-mail]]

Ugh, about:plugins is HTML, and therefore can't use our chrome entities to synchronise with the UI's direction. Ehsan therefore added an XHTML frame to do this, but he removes the frame while it's still loading, so by the time we get told about it its window no longer exists, thus the error message.

What might work is to skip documents that aren't loading (since we get multiple notifications and we only really care for the first one anyway). I haven't checked to see whether password manager still works if we do this though.

Comment 3

[Philip Chee]

Attachment: Patch v1.0 Skip documents that aren't loading

> +    if (aStateFlags & Components.interfaces.nsIWebProgressListener.STATE_STOP &&
> +        aStateFlags & Components.interfaces.nsIWebProgressListener.STATE_IS_DOCUMENT)
When I load about:plugins aStateFlags is 0x20010 which corresponds to STATE_STOP + STATE_IS_DOCUMENT

Also ports TOOLKIT patch from Bug 762593 - Add logic for validating if password input fields are insecure.

Comment 4

[[e-mail]]

Comment on attachment 8598148 [details] [diff] [review]
Patch v1.0 Skip documents that aren't loading

>+    if (aStateFlags & Components.interfaces.nsIWebProgressListener.STATE_STOP &&
>+        aStateFlags & Components.interfaces.nsIWebProgressListener.STATE_IS_DOCUMENT)
>+      return;
Actually there is a suitably named property on the web progress object...

>+    aWebProgress.DOMWindow.addEventListener("DOMFormHasPassword", function(event) {
>+      InsecurePasswordUtils.checkForInsecurePasswords(event.target);
>+    });
[Bah, this one uses event.target instead of event?]
Please make a global function, it doesn't need to be in this scope.

What's the UI for this?

Comment 5

[Philip Chee]

Attachment: Patch v2.0 Check for isLoadingDocument

>>+        aStateFlags & Components.interfaces.nsIWebProgressListener.STATE_IS_DOCUMENT)
>>+      return;
> Actually there is a suitably named property on the web progress object...
Oops. Fixed.

>>+    aWebProgress.DOMWindow.addEventListener("DOMFormHasPassword", function(event) {
>>+      InsecurePasswordUtils.checkForInsecurePasswords(event.target);
>>+    });
> [Bah, this one uses event.target instead of event?]
> Please make a global function, it doesn't need to be in this scope.
> 
> What's the UI for this?
Bug 947471 was filed to provide a proper UI.

Currently the UI is here:
https://hg.mozilla.org/mozilla-central/rev/a46b44af697f#l2.51
And here:
https://hg.mozilla.org/mozilla-central/rev/2b03e95df6d3#l8.2

Comment 6

[[e-mail]]

Comment on attachment 8600602 [details] [diff] [review]
Patch v2.0 Check for isLoadingDocument

>   onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus) {
>+    if (!aWebProgress.isLoadingDocument)
>+      return;
Nit: blank line

>     aWebProgress.DOMWindow.addEventListener("DOMFormHasPassword", onFormPassword, true);
>+    aWebProgress.DOMWindow.addEventListener("DOMFormHasPassword", checkForInsecurePasswords, true);
>     aWebProgress.DOMWindow.addEventListener("DOMAutoComplete", onUsernameInput, true);
>     aWebProgress.DOMWindow.addEventListener("change", onUsernameInput, true);
Nit: onFormPassword and onUsernameInput are linked; keep those lines together.

Comment 7

[Philip Chee]

Attachment: Patch v3.0 include port of Bug 1149975

Updated patch also includes port of Bug 1149975 (Part 1 of 2 - Handle visibility of the login fill doorhanger anchor).

I put the pageshow event handler in navigator.js because in nsSuiteGlue.js I kept getting "event.target is undefined" errors.

I also put the message listener in navigator:
> +  window.messageManager
> +        .addMessageListener("RemoteLogins:fillForm", (message) => {
> +    LoginManagerContent.receiveMessage(message, content);
> +  });

Comment 8

[Philip Chee]

Also, I asked a question in 1149975 Comment 40
https://bugzilla.mozilla.org/show_bug.cgi?id=1149975#c40

Comment 9

[[e-mail]]

(In reply to Philip Chee from comment #7)
> Updated patch also includes port of Bug 1149975 (Part 1 of 2 - Handle
> visibility of the login fill doorhanger anchor).
> 
> I put the pageshow event handler in navigator.js because in nsSuiteGlue.js I
> kept getting "event.target is undefined" errors.
> 
> I also put the message listener in navigator:
> > +  window.messageManager
> > +        .addMessageListener("RemoteLogins:fillForm", (message) => {
> > +    LoginManagerContent.receiveMessage(message, content);
> > +  });

I don't think navigator is the right place for this because the login manager is sending the message to the content script. (And I guess the pageshow event handler might as well go there too.)

Comment 10

[[e-mail]]

Comment on attachment 8607201 [details] [diff] [review]
Patch v3.0 include port of Bug 1149975

>+function onDOMFormHasPassword(event) {
>+  var content = event.target.ownerDocument.defaultView;
This is not quite right, they want event.target.ownerDocument.defaultView.top

>+    aWebProgress.DOMWindow.addEventListener("DOMFormHasPassword", onDOMFormHasPassword, true);
>     aWebProgress.DOMWindow.addEventListener("DOMAutoComplete", onUsernameInput, true);
>     aWebProgress.DOMWindow.addEventListener("change", onUsernameInput, true);
>+    aWebProgress.DOMWindow.addEventListener("DOMFormHasPassword", checkForInsecurePasswords, true);
Now that you need a separate function for both login manager and insecure password utils, might as well combine the two.

Comment 11

[Philip Chee]

Attachment: Patch v5.0 use content frame script

> I don't think navigator is the right place for this because the login 
> manager is sending the message to the content script. (And I guess the 
> pageshow event handler might as well go there too.)
In this patch I've switched to a content script containing the message listener and the pageshow event handler.

>>+function onDOMFormHasPassword(event) {
>>+  var content = event.target.ownerDocument.defaultView;
> This is not quite right, they want event.target.ownerDocument.defaultView.top
Fixed.

>>+    aWebProgress.DOMWindow.addEventListener("DOMFormHasPassword", onDOMFormHasPassword, true);
>>     aWebProgress.DOMWindow.addEventListener("DOMAutoComplete", onUsernameInput, true);
>>     aWebProgress.DOMWindow.addEventListener("change", onUsernameInput, true);
>>+    aWebProgress.DOMWindow.addEventListener("DOMFormHasPassword", checkForInsecurePasswords, true);
> Now that you need a separate function for both login manager and insecure password utils, might as well combine the two.
Fixed.

Comment 12

[[e-mail]]

Comment on attachment 8610865 [details] [diff] [review]
Patch v5.0 use content frame script

>+  var mm = window.getGroupMessageManager("browsers");
>+  mm.loadFrameScript("chrome://navigator/content/content.js", true);
I think we should start by loading the frame script into all content frames using the global message manager, so that it covers sidebars and RSS feeds and such.

>+function onDOMFormHasPassword(event) {
>+  var content = event.target.ownerDocument.defaultView.top;
>+  LoginManagerContent.onDOMFormHasPassword(event, content);
>+  InsecurePasswordUtils.checkForInsecurePasswords(event.target);
>+}
Doesn't this belong in the content script now?

>   onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus) {
>-    aWebProgress.DOMWindow.addEventListener("DOMFormHasPassword", onFormPassword, true);
>+    if (!aWebProgress.isLoadingDocument)
>+      return;
>+
>+    aWebProgress.DOMWindow.addEventListener("DOMFormHasPassword", onDOMFormHasPassword, true);
>     aWebProgress.DOMWindow.addEventListener("DOMAutoComplete", onUsernameInput, true);
>     aWebProgress.DOMWindow.addEventListener("change", onUsernameInput, true);
>   },
[Probably all of this does too.]

Comment 13

[Philip Chee]

Attachment: Patch v6.0 moved almost everything to content.js

> I think we should start by loading the frame script into all content 
> frames using the global message manager, so that it covers sidebars and 
> RSS feeds and such.
Done.

>>+function onDOMFormHasPassword(event) {
> Doesn't this belong in the content script now?
Moved to content.js

>>   onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus) {
>>-    aWebProgress.DOMWindow.addEventListener("DOMFormHasPassword", onFormPassword, true);
>>+    if (!aWebProgress.isLoadingDocument)
>>+      return;
>>+
>>+    aWebProgress.DOMWindow.addEventListener("DOMFormHasPassword", onDOMFormHasPassword, true);
>>     aWebProgress.DOMWindow.addEventListener("DOMAutoComplete", onUsernameInput, true);
>>     aWebProgress.DOMWindow.addEventListener("change", onUsernameInput, true);
>>   },
> [Probably all of this does too.]
Moved to content.js

Comment 15

[[e-mail]]

Comment on attachment 8614277 [details] [diff] [review]
Patch v6.0 moved almost everything to content.js

A password got filled in so I assume that means it's working.

>   onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus) {
>-    aWebProgress.DOMWindow.addEventListener("DOMFormHasPassword", onFormPassword, true);
>-    aWebProgress.DOMWindow.addEventListener("DOMAutoComplete", onUsernameInput, true);
>-    aWebProgress.DOMWindow.addEventListener("change", onUsernameInput, true);
>+    if (!aWebProgress.isLoadingDocument)
>+      return;
>   },
This isn't doing anything any more so remove it and remove NOTIFY_STATE_DOCUMENT from _init(). r=me with that fixed.

Comment 16

[Ian Neal]

Once new patch is there, a=me for CLOSED TREE

Comment 17

[Philip Chee]

http://hg.mozilla.org/comm-central/rev/33c0832f86e0