Bug 874690 exposed two related problems with Mozmill tests: 1. The order in which the files in RELATIVE_ROOT (=shared-modules), and probably the test files in a folder as well, are loaded, is not specified, see https://bugzilla.mozilla.org/show_bug.cgi?id=874690#c21 By itself, this is not a problem, because loaded scripts should only expect in their global object the symbols placed there by the module system, and import everything else using the mechanisms provided. 2. The loading mechanism of Mozmill allows leaking of symbols through the global object of the calling script. It uses the loadSubScript() method of mozIJSSubScriptLoader, see https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/mozIJSSubScriptLoader. As explained in http://stackoverflow.com/questions/15179265/how-do-i-use-the-targetobj-parameter-to-loadsubscript-in-firefox-extensions (2nd answer), "undeclared variables will still be created in the outer scope and the outer scope will be searched for variables that cannot be resolved in the subscript scope." It should be possible to detect and prevent such accidental leaking of symbols, which may lead to scripts working only by accident when the loading order is "nice", but not working in other cases where the loading order is "not so nice". The Stackoverflow answer suggests using Components.utils.Sandbox. But this not only prevents leaking of symbols, but also enables (unwanted) security mechanisms. So more investigation is needed before this might work.

Repeating the question from https://bugzilla.mozilla.org/show_bug.cgi?id=874690#c24: Is the copy in c-c the authoritative one, or is Mozmill coming from somewhere upstream?

I asked for help on Stackoverflow: http://stackoverflow.com/questions/16746837/mozijssubscriptloaderloadsubscript-how-to-prevent-leaking-of-symbols-without

On IRC, Mook suggested using the system principal for the sandbox, see https://developer.mozilla.org/en-US/docs/Security_check_basics#Special_principals I will give that a try later on.

I tried out using a sandbox created with the system principal, see attached patch. But unfortunately, that doesn't work: Using profile dir: /home/jens/devel/mozilla-comm-central/obj-x86_64-unknown-linux-gnu/mozilla/_tests/mozmill/mozmillprofile Timeout: bridge.set("d4add76a-c505-11e2-b2e4-4061865b3557", Components.utils.import('resource://mozmill/modules/mozmill.js')) Traceback (most recent call last): File "runtest.py", line 515, in <module> ThunderTestCLI().run() File "/home/jens/devel/mozilla-comm-central/obj-x86_64-unknown-linux-gnu/mozilla/_tests/mozmill-virtualenv/local/lib/python2.7/site-packages/mozmill/__init__.py", line 785, in run self.mozmill.start(runner=runner, profile=runner.profile) File "/home/jens/devel/mozilla-comm-central/obj-x86_64-unknown-linux-gnu/mozilla/_tests/mozmill-virtualenv/local/lib/python2.7/site-packages/mozmill/__init__.py", line 230, in start self.appinfo = self.get_appinfo(self.bridge) File "/home/jens/devel/mozilla-comm-central/obj-x86_64-unknown-linux-gnu/mozilla/_tests/mozmill-virtualenv/local/lib/python2.7/site-packages/mozmill/__init__.py", line 336, in get_appinfo mozmill = jsbridge.JSObject(bridge, mozmillModuleJs) File "/home/jens/devel/mozilla-comm-central/obj-x86_64-unknown-linux-gnu/mozilla/_tests/mozmill-virtualenv/local/lib/python2.7/site-packages/jsbridge/jsobjects.py", line 76, in __init__ name = bridge.set(name)['data'] File "/home/jens/devel/mozilla-comm-central/obj-x86_64-unknown-linux-gnu/mozilla/_tests/mozmill-virtualenv/local/lib/python2.7/site-packages/jsbridge/network.py", line 230, in set return self.run(_uuid, 'bridge.set('+', '.join([encoder.encode(_uuid), obj_name])+')') File "/home/jens/devel/mozilla-comm-central/obj-x86_64-unknown-linux-gnu/mozilla/_tests/mozmill-virtualenv/local/lib/python2.7/site-packages/jsbridge/network.py", line 198, in run raise JSBridgeDisconnectError("Connection timed out") jsbridge.network.JSBridgeDisconnectError: Connection timed out make: *** [mozmill-one] Fehler 1

Hi Mikeal, as you are the author of frame.js: Do you maybe have an idea about how to overcome the problem described in the last note? Or if you think that it cannot work at all, an explanation to that end?

This works in general. I get this message: SUMMARY-UNEXPECTED-FAIL | test-cloudfile-backend-helpers.js | test-cloudfile-backend-helpers.js::<TOP_LEVEL> EXCEPTION: os is not defined - but that's exactly the kind of relying on leaked symbols which we want to detect. @Mikeal: Sorry for bothering you.

I added missing imports in quite a number of files. I also attached a file with some remaining messages. * "os is not defined" - definitely my task * "fixIterator is not defined" - what has to be imported to get it? -> Ah, mailnews/base/util/iteratorUtils.jsm * "window is not defined", "tab is not defined" -> I'll have to have a closer look regarding WHICH window and which tab are meant here. * "folder.addMessage is not a function" - Probably not caused by me?! I'll take a look at older tbpl logs ...

(In reply to Jens Müller from comment #10) > * "folder.addMessage is not a function" - Probably not caused by me?! I'll > take a look at older tbpl logs ... The folder is created by testHelperModule.make_empty_folder(), which is defined in test_folder_display_helpers.js. There I see: > // The xpcshell test resources assume they are loaded into a single global > // namespace, so we need to help them out to maintain their delusion. > load_via_src_path('logHelper.js', testHelperModule); There is no make_empty_folder in logHelper.js ... That is in mailnews/test/resources/messageInjection.js. I read in test-folder-display-helper.js's setupModule: > // messageInjection wants a gMessageGenerator (and so do we) Well - why is that part _after_ loading messageInjection.js -> I'll try moving the load after that block. Nope, doesn't help either. Note for further testing: > make SOLO_TEST=composition/test-save-changes-on-quit.js mozmill-one is sufficient to reproduce this.

dump("Current value of folder: "+JSON.stringify(folder,function(key,value){if (key=="") return value; else return true;})+"\n"); gives: Current value of folder: {"QueryInterface":true,"messages":true,"startFolderLoading":true,"endFolderLoading":true,"updateFolder":true,"prettiestName":true,"folderURL":true,"showDeletedMessages":true,"server":true,"isServer":true,"canSubscribe":true,"canFileMessages":true,"noSelect":true,"imapShared":true,"canDeleteMessages":true,"canCreateSubfolders":true,"canRename":true,"canCompact":true,"rootFolder":true,"getFilterList":true,"setFilterList":true,"getEditableFilterList":true,"setEditableFilterList":true,"ForceDBClosed":true,"closeAndBackupFolderDB":true,"Delete":true,"deleteSubFolders":true,"propagateDelete":true,"recursiveDelete":true,"createSubfolder":true,"addSubfolder":true,"createStorageIfMissing":true,"compact":true,"compactAll":true,"compactAllOfflineStores":true,"emptyTrash":true,"rename":true,"renameSubFolders":true,"generateUniqueSubfolderName":true,"updateSummaryTotals":true,"summaryChanged":true,"getNumUnread":true,"getTotalMessages":true,"hasNewMessages":true,"firstNewMessage":true," clearNewMessages":true,"expungedBytes":true,"deletable":true,"displayRecipients":true,"manyHeadersToDownload":true,"knowsSearchNntpExtension":true,"allowsPosting":true,"relativePathName":true,"sizeOnDisk":true,"username":true,"hostname":true,"setFlag":true,"clearFlag":true,"getFlag":true,"toggleFlag":true,"onFlagChange":true,"flags":true,"getFolderWithFlags":true,"getFoldersWithFlags":true,"listFoldersWithFlags":true,"isSpecialFolder":true,"getUriForMsg":true,"deleteMessages":true,"copyMessages":true,"copyFolder":true,"copyFileMessage":true,"acquireSemaphore":true,"releaseSemaphore":true,"testSemaphore":true,"locked":true,"getNewMessages":true,"writeToFolderCache":true,"charset":true,"charsetOverride":true,"biffState":true,"getNumNewMessages":true,"setNumNewMessages":true,"gettingNewMessages":true,"filePath":true,"baseMessageURI":true,"generateMessageURI":true,"addMessageDispositionState":true,"markMessagesRead":true,"markAllMessagesRead":true,"markMessagesFlagged":true,"markThreadRead":true," setLabelForMessages":true,"msgDatabase":true,"getBackupMsgDatabase":true,"removeBackupMsgDatabase":true,"openBackupMsgDatabase":true,"getDBFolderInfoAndDB":true,"GetMessageHeader":true,"supportsOffline":true,"shouldStoreMsgOffline":true,"hasMsgOffline":true,"getOfflineFileStream":true,"GetOfflineMsgFolder":true,"getOfflineStoreOutputStream":true,"getMsgInputStream":true,"offlineStoreInputStream":true,"DownloadMessagesForOffline":true,"getChildWithURI":true,"downloadAllForOffline":true,"enableNotifications":true,"isCommandEnabled":true,"matchOrChangeFilterDestination":true,"confirmFolderDeletionForFilter":true,"alertFilterChanged":true,"throwAlertMsg":true,"getStringWithFolderNameFromBundle":true,"notifyCompactCompleted":true,"compareSortKeys":true,"getSortKey":true,"retentionSettings":true,"downloadSettings":true,"callFilterPlugins":true,"sortOrder":true,"dBTransferInfo":true,"getStringProperty":true,"setStringProperty":true,"lastMessageLoaded":true,"URI":true,"name":true,"prettyName":true,"abbreviatedName": true,"parent":true,"subFolders":true,"hasSubFolders":true,"numSubFolders":true,"isAncestorOf":true,"containsChildNamed":true,"getChildNamed":true,"findSubFolder":true,"AddFolderListener":true,"RemoveFolderListener":true,"NotifyPropertyChanged":true,"NotifyIntPropertyChanged":true,"NotifyBoolPropertyChanged":true,"NotifyPropertyFlagChanged":true,"NotifyUnicharPropertyChanged":true,"NotifyItemAdded":true,"NotifyItemRemoved":true,"NotifyFolderEvent":true,"descendants":true,"ListDescendants":true,"Shutdown":true,"inVFEditSearchScope":true,"setInVFEditSearchScope":true,"copyDataToOutputStreamForAppend":true,"copyDataDone":true,"setJunkScoreForMessages":true,"applyRetentionSettings":true,"fetchMsgPreviewText":true,"addKeywordsToMessages":true,"removeKeywordsFromMessages":true,"getMsgTextFromStream":true,"convertMsgSnippetToPlainText":true,"customIdentity":true,"getProcessingFlags":true,"orProcessingFlags":true,"andProcessingFlags":true,"getInheritedStringProperty":true,"setForcePropertyEmpty":true," getForcePropertyEmpty":true,"msgStore":true,"nsMsgBiffState_NewMail":true,"nsMsgBiffState_NoMail":true,"nsMsgBiffState_Unknown":true,"nsMsgDispositionState_None":true,"nsMsgDispositionState_Replied":true,"nsMsgDispositionState_Forwarded":true,"allMessageCountNotifications":true} -> all kinds of functions, but no "addMessage" ...

The folder object is created by calling the method createLocalSubfolder of the nsIMsgLocalMailFolder interface. This returns an nsIMsgFolder. nsIMsgFolder does NOT have a method addMessage. I found this: > mailnews/test/resources/messageGenerator.js:function addMessagesToFolder (aMessages, aFolder) which might be a replacement. I have looked at it, and it gives the clue on what was missing: > let localFolder = aFolder.QueryInterface(Ci.nsIMsgLocalMailFolder); > for (let [, message] in Iterator(aMessages)) > localFolder.addMessage(message.toMboxString()); -- a simple QueryInterface ... But how could this ever have worked? Or did it even ever work?

This fixes the last problem. Now the next similar problem occurs: SUMMARY-UNEXPECTED-FAIL | test-save-changes-on-quit.js | test-save-changes-on-quit.js::setupModule EXCEPTION: synMsg.toMboxString is not a function at: messageInjection.js line 697 add_sets_to_folders messageInjection.js 697 add_message_to_folder test-folder-display-helpers.js 448 setupModule test-save-changes-on-quit.js 40 Runner.prototype.wrapper frame.js 580 Runner.prototype._runTestModule frame.js 629 Runner.prototype.runTestModule frame.js 701 Runner.prototype.runTestFile frame.js 534 runTestFile frame.js 713 Bridge.prototype._execFunction server.js 179 Bridge.prototype.execFunction server.js 183 This really is a hydra ...

The problem now is: How do I turn a string with a message source, as created by http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/composition/test-save-changes-on-quit.js#45 into a SyntheticMessage as expected by http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-folder-display-helpers.js#444 ?

OK - the easiest way is probably to rewrite http://mxr.mozilla.org/comm-central/ident?i=msgSource so that it returns a SyntheticMessage by calling http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-folder-display-helpers.js#424 - format of aArgs is explained here: http://mxr.mozilla.org/comm-central/source/mailnews/test/resources/messageGenerator.js#813

Hi Andrew, do you have an idea how to fix this problem? EXCEPTION: messageSet is not defined at: messageInjection.js line 713 http://mxr.mozilla.org/comm-central/source/mailnews/test/resources/messageInjection.js#713 Best regards, Jens

I think the general problem as reported in comment 0 is real so I confirm this bug. I think we should import Services and mailServices globally before test execution and then import all other modules explicitly in each test as needed, as Jens proposes.

With the last patch, it works. I have to look carefully at add_sets_to_folders again. Maybe I broke something by calling it in the wrong way ... (In reply to Jens Müller from comment #10) > * "window is not defined", "tab is not defined" -> I'll have to have a > closer look regarding WHICH window and which tab are meant here. These are the remaining problems ... And I now see the following: SUMMARY-UNEXPECTED-FAIL | test-tabmail-dragndrop.js | test-tabmail-dragndrop.js::test_tab_recentlyClosed EXCEPTION: Timeout waiting for popup to open at: utils.js line 447 TimeoutError utils.js 447 waitFor utils.js 485 wait_for_popup_to_open test-folder-display-helpers.js 1371 _synthesizeRecentlyClosedMenu test-tabmail-dragndrop.js 301 test_tab_recentlyClosed test-tabmail-dragndrop.js 355 Runner.prototype.wrapper frame.js 585 Runner.prototype._runTestModule frame.js 655 Runner.prototype.runTestModule frame.js 701 Runner.prototype.runTestDirectory frame.js 525 runTestDirectory frame.js 707 Bridge.prototype._execFunction server.js 179 Bridge.prototype.execFunction server.js 183 It's not in the TBPL logs, but I don't know exactly if it was caused by my changes.

(In reply to Jens Müller from comment #21) > (In reply to Jens Müller from comment #10) > > * "window is not defined", "tab is not defined" -> I'll have to have a > > closer look regarding WHICH window and which tab are meant here. > > These are the remaining problems ... Reproducible with just > make SOLO_TEST=tabmail/test-tabmail-customize.js mozmill-one

The questions regarding this issue now are: 1. EventUtils.js apparently expects a window object it can use as fallback. Is there any way this can work? Who is supposed to put it in EventUtils.js's scope? 2. The call is from controller.js line 515. Apparently, element.ownerDocument.defaultView evaluates to null there. Is this critical? It happens during teardownModule ... Maybe a timing issue?

(In reply to :aceman from comment #19) > I think we should import Services and mailServices globally before test > execution I have now imported them explicitly where they are needed. My first idea to do that globally is through the loadFile function in frame.js: 1. Import them at the beginning of frame.js: > Cu.import("resource:///modules/mailServices.js"); > Cu.import("resource://gre/modules/Services.jsm"); 2. In loadFile, explicitly inject them into the scope object of the file to be loaded: > module.Services = Services; > module.MailServices = MailServices; But is it really a good idea to hard-code this (at this place)? Especially the second line would make Mozmill more mailnews-specific ... If someone knows a better way of defining which components to load and which scripts to inject into the scope object of all loaded files, such an idea would be very welcome.

Probably do not do it in the base mozmill infrastructure. Can you find a place that only imports it when tests under /mail/test/mozmill are run?

But the global inclusion of *Services was only my opinion, as they are used in many tests so their inclusions is duplicated code. But better ask mconley if we really want this.

loadFile in frame.js already injects mozmill and several other symbol into the scope of all loaded files. For example, test-window-helpers.js does this: > var mozmill = {}; > Cu.import('resource://mozmill/modules/mozmill.js', mozmill); Also probably concerns other files, and other symbols such as elib. But maybe removing these unnecessary imports should be a separate bug.

(In reply to Jens Müller from comment #23) > The questions regarding this issue now are: > 1. EventUtils.js apparently expects a window object it can use as fallback. > Is there any way this can work? Who is supposed to put it in EventUtils.js's > scope? > 2. The call is from controller.js line 515. Apparently, > element.ownerDocument.defaultView evaluates to null there. Is this critical? > It happens during teardownModule ... Maybe a timing issue? This already fails currently on tryservers: TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/tabmail/test-tabmail-customize.js | test-tabmail-customize.js::teardownModule So not part of this bug.

(In reply to Jens Müller from comment #21) > And I now see the following: > > SUMMARY-UNEXPECTED-FAIL | test-tabmail-dragndrop.js | > test-tabmail-dragndrop.js::test_tab_recentlyClosed > EXCEPTION: Timeout waiting for popup to open I now learnt that this happens randomly on Linux. So this is also not part of this bug. IMO, I am done now. Everything else can go to separate new bugs.

(In reply to Jens Müller from comment #27) > loadFile in frame.js already injects mozmill and several other symbol into > the scope of all loaded files. See bug 876250.

Ted, Henrik, Clint, Jeff: Can you have a look whether this is relevant to Mozmill in general?

(In reply to :aceman from comment #25) > Probably do not do it in the base mozmill infrastructure. Can you find a > place that only imports it when tests under /mail/test/mozmill are run? My idea would be to define some directory (possibly in a config file), and all files (one will probably suffice for the start) within it get loaded automatically before anything else, and all symbols get injected into all the other scripts. This requires significant effort and should be done in a separate bug. Plus, it should probably be coordinated with the Autotest team ...

(In reply to Jens Müller from comment #7) > Created attachment 754080 [details] [diff] [review] > Use Sandbox created with the system principal I haven't checked in detail yet but as it looks like it sounds useful, yes. We should get this fixed in Mozmill base. Can you please file a bug for it with all the necessary information? Thanks.

(In reply to Henrik Skupin (:whimboo) from comment #33) > We should get this fixed in Mozmill base. Can you please file a bug for it > with all the necessary information? Thanks. Bug 876399.

This will likely be fixed in Mozmill 2.0 and maybe also Mozmill 1.5 soon. The question is how to proceed ... Possible alternatives are: 1. Wait until Mozmill 2.0 is released and switch to that version. This will require rewriting the module dependencies: Mozmill 2.0 uses require, the old way with MODULE_NAME, MODULE_REQUIRES and RELATIVE_ROOT is no longer supported. 2. Wait until the next Mozmill 1.5.x. I guess (but would have to check) that the old way of expressing module dependencies is still supported there ... Maybe that update will also break other things ... 3. For now, fix the issue locally. I suppose the above patch which directly modifies frame.js should be accompanied by a patch file to make clear what the difference to the upstream version of Mozmill is.

(In reply to Jens Müller (:tessarakt) from comment #35) > 1. Wait until Mozmill 2.0 is released and switch to that version. This will > require rewriting the module dependencies: Mozmill 2.0 uses require, the old > way with MODULE_NAME, MODULE_REQUIRES and RELATIVE_ROOT is no longer > supported. I don't think you want to switch at this point. There is even more work you would have to do. But FYI we have not removed the old module loader code. It will still be present in Mozmill 2.0. > 2. Wait until the next Mozmill 1.5.x. I guess (but would have to check) > that the old way of expressing module dependencies is still supported there > ... Maybe that update will also break other things ... Same as above. And no, it should not break other stuff. But you are highly advised to test the patch before we release 1.5.22. I would love to get the feedback from your whole test suite. > 3. For now, fix the issue locally. I suppose the above patch which directly > modifies frame.js should be accompanied by a patch file to make clear what > the difference to the upstream version of Mozmill is. Up to you if you can live with the merge conflicts later.

(In reply to :aceman from comment #26) > But the global inclusion of *Services was only my opinion, as they are used > in many tests so their inclusions is duplicated code. But better ask mconley > if we really want this. The TB Mozmill tests already have quite a bit of magic injected into them. Cc, Ci, Cu, and Cr are magically available, for example. I honestly prefer *less* magic over *more*. I prefer seeing explicit imports rather than magically injected symbols. So, IMO, I'd rather the importing of MailServices and Services continue to be explicit.

Bug 876399 has landed. My recommendation would be to wait for the next Mozmill 1.5 release and then do the update together with the necessary fixes.

Mconley, can you tell us how the new mozmill comes to comm-central?

Comment on attachment 754080 [details] [diff] [review] Use Sandbox created with the system principal r=me based on the fact this is already in the upstream mozmill code.

(In reply to :aceman from comment #39) > Mconley, can you tell us how the new mozmill comes to comm-central? When a release comes out, someone needs to import/copy the release to comm-central, run tests and fix any failures. The result just needs to be attached on a new bug and reviewed by an appropriate peer (note: mozmill upgrade & test fix patches separate please).

Note, this needs landing with bug 874690 once that one has got review.

This fixes the one remaining undefined symbol standard8 found in bug 874690 comment 36.

https://hg.mozilla.org/comm-central/rev/3a43daef7bb2 https://hg.mozilla.org/comm-central/rev/38a98ab05a36 https://hg.mozilla.org/comm-central/rev/ebd070933157

I've just backed out the main patch due to this having caused plugin tests failures. I didn't pick this up at the time of landing due to the rest of the orange, and wrong assuming it was a different bug. https://hg.mozilla.org/comm-central/rev/9e5b10914063 Example failure: https://tbpl.mozilla.org/php/getParsedLog.php?id=25605729&tree=Thunderbird-Trunk#error0 TEST-START | /home/cltbld/talos-slave/test/build/mozmill/content-tabs/test-plugin-crashing.js | setupTest Step Pass: {"function": "controller.click()"} ###!!! [Parent][RPCChannel] Error: Channel error: cannot send/recv TEST-PASS | /home/cltbld/talos-slave/test/build/mozmill/content-tabs/test-plugin-crashing.js | test-plugin-crashing.js::setupTest TEST-START | /home/cltbld/talos-slave/test/build/mozmill/content-tabs/test-plugin-crashing.js | test_crashed_plugin_notification_inline Step Pass: {"function": "controller.waitFor()"} Test Failure: Timed out waiting for plugin status div to appear TEST-START | /home/cltbld/talos-slave/test/build/mozmill/content-tabs/test-plugin-crashing.js | teardownTest TEST-PASS | /home/cltbld/talos-slave/test/build/mozmill/content-tabs/test-plugin-crashing.js | test-plugin-crashing.js::teardownTest TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/content-tabs/test-plugin-crashing.js | test-plugin-crashing.js::test_crashed_plugin_notification_inline I'm guessing there's some sort of scoping issue, but I couldn't see it at a brief inspection.

Yes, that seems to have worked. Congrats to first green Z in a long while! :) https://tbpl.mozilla.org/php/getParsedLog.php?id=25651398&tree=Thunderbird-Trunk

Japp, test-plugin-crashing.js::test_crashed_plugin_notification_inline fails with the patch applied. But for test_crashed_plugin_notification_bar, I also see some error messages (with the patch applied): ###!!! [Parent][RPCChannel] Error: Channel error: cannot send/recv WARNING: Failed to send message!: file /home/jens/devel/comm-central/mozilla/dom/plugins/ipc/PluginScriptableObjectParent.cpp, line 168 and WARNING: We should have hit the document element...: file /home/jens/devel/comm-central/mozilla/layout/xul/base/src/nsBoxObject.cpp, line 175 I looked at it without the patch applied - same errors/warnings, and also: ###!!! ASSERTION: Component Manager being held past XPCOM shutdown.: 'cnt == 0', file /home/jens/devel/comm-central/mozilla/xpcom/build/nsXPComInit.cpp, line 717 It also seems to that the main area of the Thunderbird window is resized, but the paint of the widget taking the space in the lower part never happens (also without the patch applied!)

Apparently sandboxes are harder to use than it looks at first glance ... Maybe it has to do with https://developer.mozilla.org/en-US/docs/Safely_accessing_content_DOM_from_chrome . setupTest() accesses retrieves the wrappedJSObject property of the content window. This is then used to cause the plugin to crash. When the plugin crashes, the property mozNoPluginCrashedNotification is set on the document. This property is accessed (in the waitFor callback) via the non-wrapped document. Maybe that is the problem? However, using gTabDoc.mozNoPluginCrashedNotification makes no difference. I also found this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=787070 "So the problem is if you add an expando (from chrome side) on the xray of let's say Node.prototype (of the content) you won't find the property on the xray of any Node instances." While I don't know what an "expando" is, and I am not sure whether it is added from chrome side, this at least sounds related ... Maybe someone with a deeper understanding of these issue can comment on this ...

I just tried - mc.waitFor(function() gContentWindow.document.mozNoPluginCrashedNotification, + mc.waitFor(function() plugin.ownerDocument.mozNoPluginCrashedNotification, "Timed out waiting for plugin status div to appear"); -- but that doesn't even work without the sandbox patch ...

For the record: gContentWindow.document is a [object XrayWrapper [object HTMLDocument]]. That gives me some new clue to search for ...

Looking at the comments in http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/XrayWrapper.cpp, it seems that sandboxes never see anyone else's expando properties on wrapped objects ...

In http://mxr.mozilla.org/comm-central/source/mail/base/content/plugins.js#641, doc is an [object XrayWrapper [object HTMLDocument]]. So does this mean that mozNoPluginCrashedNotification is only set on the wrapper? Replacing that line by XPCNativeWrapper.unwrap(doc).mozNoPluginCrashedNotification = true; plus mc.waitFor(function() XPCNativeWrapper.unwrap(gContentWindow.document).mozNoPluginCrashedNotification, "Timed out waiting for plugin status div to appear"); in the test script does it for me. But this is not a real solution yet, right?

To summarize my understanding: Both the plugin code and the test access the document object through an XrayWrapper. The plugin code thus sets the mozNoPluginCrashedNotification on the wrapper, not the underlying object. This is "no problem" when the test does not run in a sandbox, because both pieces of code share the same XrayWrapper. But sandboxed code always gets its own XrayWrapper, so then it suddenly becomes a problem. When looking at this code: https://mxr.mozilla.org/comm-central/source/mail/test/mozmill/content-tabs/test-plugin-crashing.js#230, it seems to me that "Timed out waiting for plugin status div to appear" does not really describe what is happening here. The code does not look for any "plugin status div". Rather, it waits for a flag to be set. And this flag is set together with a call to hideNotificationBar(). IMO, looking for some internal flags of other code is wrong anyway. Rather, it should look for the notification bar and wait until it is no longer there ... Shouldn't be too difficult. In fact, I think I was almost there on my Linux machine, but forgot a ! operator ...

OK, I think this is working. I added test output to test_crashed_plugin_notification_bar. There, mc.tabmail.selectedTab.browser.parentNode.getNotificationWithValue("plugin-crashed") is an [object XULElement]. In test_crashed_plugin_notification_inline, this expression is always null. There is no notification before the plugin is crashed, and there must not be a notification after it has been crashed. The "waitFor" function call confused me for a while - I thought a notification was supposed to exist and then disappear ...

Can someone please push attachments 783326 and 754080 to tryserver, please?

Hi Archaeopteryx, apparently the bustage is fixed. Can you please push to tryserver again? Thanks, Jens

Pushed to Thunderbird-Try as https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=94faa086cd89

Test fails, but apparently at a later point than before: TEST-START | /home/cltbld/talos-slave/test/build/mozmill/content-tabs/test-plugin-unknown.js | test_unknown_plugin_notification_bar Step Pass: {"function": "controller.click()"} Test Failure: Timeout waiting for alert dispatching InstallBrowserTheme TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/content-tabs/test-plugin-unknown.js | test-plugin-unknown.js::test_unknown_plugin_notification_bar

That should now be fixed (by https://hg.mozilla.org/comm-central/rev/e9478b7910d0)

Comment on attachment 783326 [details] [diff] [review] Fix issue with inaccessible expando property in plugin test Looks good, thanks.

Fix issue with inaccessible expando property in plugin test (now with proper patch header)

https://hg.mozilla.org/comm-central/rev/b08eaa87c337 https://hg.mozilla.org/comm-central/rev/f97b99aa324b