681461 - Do not unwrap the window and opener when handling modal dialogs (breaks testSubmitUnencryptedInfoWarning.js)

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

Test: Functional::Security::testSubmitUnencryptedInfoWarning
Fail: Disconnect Error: Application unexpectedly closed
Branch: default, aurora

Comment 1

[u279076]

Attachment: Patch v1.0 (nightly/aurora) [checked-in]

This patch skips the test -- only applies for Nightly and Aurora.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 555200 [details] [diff] [review]
Patch v1.0 (nightly/aurora) [checked-in]

I believe both disconnects in the last couple of days are related to each other. We should really get those investigated.

Comment 3

[u279076]

Comment on attachment 555200 [details] [diff] [review]
Patch v1.0 (nightly/aurora) [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/e658459426f6 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/77206068e42a (aurora)

Comment 4

[u279076]

Remus, can you have a look at this bug and see if you can reproduce it? If not I will have to look into it next week on QA-Horus.

Comment 5

[Remus Pop (:RemusPop)]

I was able to reproduce it by running this test on the latest nightly and aurora. It seems that the test cannot get over the dialog that appears. Even if I manually click continue the test fails.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Which dialog do you see? Can you attach a screenshot? Is that a modal dialog? Do we expect such a dialog in the test?

Comment 7

[Remus Pop (:RemusPop)]

Attachment: screenshot of dialog

We expect this dialog in the test.

Comment 8

[Remus Pop (:RemusPop)]

Running this test stops us to:
found = (mozmill.utils.getChromeWindow(opener) == this._opener);
which is part of the findWindow function in the modal dialog observer.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Please do a regression test with Nightly builds of Firefox to find out for which build it started to happen. The same code base works fine with Firefox Beta so there is a regression in Firefox.

Comment 10

[Remus Pop (:RemusPop)]

Found the regression range to be:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a627b24e684e&tochange=cef1817c3b13

This appears to be related to bug 626455.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Remus Pop from comment #10)
> This appears to be related to bug 626455.

This is Panorama related which we make not use of in this test. So I can't believe it's directly related. I will do a hg bisect to narrow down the regression range.

Comment 12

[Remus Pop (:RemusPop)]

The regression range got narrowed down to:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4df55cccaff4&tochange=fe48bbfeff94

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

When running with a debug build I get the following exception:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'Error: Permission denied for <https://www.mozilla.org> to create wrapper for object of class UnnamedClass' when calling method: [nsIObserver::observe]"  nsresult: "0x8057001c (NS_ERROR_XPC_JS_THREW_JS_OBJECT)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: no]
************************************************************

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Henrik Skupin (:whimboo) from comment #13)
> ************************************************************
> * Call to xpconnect wrapped JSObject produced this error:  *
> [Exception... "'Error: Permission denied for <https://www.mozilla.org> to
> create wrapper for object of class UnnamedClass' when calling method:
> [nsIObserver::observe]"  nsresult: "0x8057001c
> (NS_ERROR_XPC_JS_THREW_JS_OBJECT)"  location: "native frame :: <unknown
> filename> :: <TOP_LEVEL> :: line 0"  data: no]
> ************************************************************

Blake, could that be a regression from your patch on bug 674182? Those errors always happen when a website spawns a modal dialog.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Henrik Skupin (:whimboo) from comment #14)
> Blake, could that be a regression from your patch on bug 674182? Those
> errors always happen when a website spawns a modal dialog.

Ok, so my assumption was correct. It is a regression from this mentioned bug:

The first bad revision is:
changeset:   73447:83535f44db38
user:        Blake Kaplan <[e-mail]>
date:        Wed Jul 27 12:29:26 2011 -0700
summary:     Fix bug 674182. r=jst

/CC some folks who probably can help as long as Blake is away.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The code we are using to handle modal dialogs can be found here:
http://hg.mozilla.org/qa/mozmill-tests/file/default/lib/modal-dialog.js

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

So, looking at that code, I see that you're using something called unwrapNode. I can't find the implementation of that function, but from bug 629832, it appears to be a function that takes a (possibly) Xray-wrapped object and "unwraps" it by calling XPCNativeWrapper.unwrap.

I don't understand why you're using that function in modal-dialog.js. It doesn't appear that you're using any sort of XBL on the content node and in this case, security wrappers are actually doing their job. When you unwrap an Xray wrapper, you're explicitly saying that "everything that happens beyond this point should happen as though chrome is doing it." Therefore, what I assume is happening is that you're QIing to something that has to create an XPCWrappedNative, but since we're going though the .wrappedJSObject of something, XPConnect doesn't think you're chrome and it throws.

By the way, where are the implementations of the mozmill.utils functions?

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Blake Kaplan (:mrbkap) from comment #17)
> So, looking at that code, I see that you're using something called
> unwrapNode. I can't find the implementation of that function, but from bug
> 629832, it appears to be a function that takes a (possibly) Xray-wrapped
> object and "unwraps" it by calling XPCNativeWrapper.unwrap.

The method has been moved to Mozmill native and is available here:
https://github.com/mozautomation/mozmill/blob/hotfix-1.5/mozmill/mozmill/extension/resource/modules/utils.js#L401

> I don't understand why you're using that function in modal-dialog.js. It
> doesn't appear that you're using any sort of XBL on the content node and in
> this case, security wrappers are actually doing their job. When you unwrap
> an Xray wrapper, you're explicitly saying that "everything that happens
> beyond this point should happen as though chrome is doing it." Therefore,
> what I assume is happening is that you're QIing to something that has to
> create an XPCWrappedNative, but since we're going though the
> .wrappedJSObject of something, XPConnect doesn't think you're chrome and it
> throws.

So you say that we do not have to unwrap those nodes. I can remember that I have had problems with it when I have implemented that code. Now when I do not call unwrap it works fine and I don't see any problem across all of our tests which make use of the modal dialog module.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Fix v1.0

Blake, is that what you expect to see? At least it is working fine for our tests.

Comment 20

[u415141]

A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/17805221

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 557805 [details] [diff] [review]
Fix v1.0

Yeah. For most use cases, .wrappedJSObect is totally unnecessary. The only reason that you'd want to use it would be if you want to interact directly with page JS (e.g. call a function defined by a page) or interact directly with XBL attached to an element in the page.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Fix v1.1 [checked-in]

Thanks Blake! I will remember that. Here a slightly updated patch, to clean-up the code a bit more.

Comment 23

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 557943 [details] [diff] [review]
Fix v1.1 [checked-in]

r+ looks fine. Maybe should echo the landing comment to 675914 too, to make it clear what fix closed it out.

Are you taking care of re-enabling the tests in the Litmus run?

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Fix (backport) [checked-in]

Backport for beta, release, and hopefully 1.9.2 too.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 557943 [details] [diff] [review]
Fix v1.1 [checked-in]

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/9845dbe7a9cf (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/4141d370e16b (aurora)

Comment 26

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 557965 [details] [diff] [review]
Fix (backport) [checked-in]

Looks fine, r+

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 557965 [details] [diff] [review]
Fix (backport) [checked-in]

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/53a5c1403898 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/b1940f446522 (release)

I don't intend to land it on 1.9.2.

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This test were never disabled on Litmus in the interim. :/ So nothing to change on that end, and we are done.