Closed Bug 1557233 Opened 5 years ago Closed 5 years ago

TEST-UNEXPECTED-FAIL | [snip]/mozmill/newmailaccount/test-newmailaccount.js

Categories

(Thunderbird :: General, defect)

Product:
Thunderbird
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(thunderbird68 fixed, thunderbird69 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 69.0
Tracking Flags:
Tracking Status
thunderbird68 --- fixed
thunderbird69 --- fixed

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

References

(Regression)

Details

(Keywords: regression, Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test])

Attachments

(3 files, 4 obsolete files)

set the pref
1.34 KB, patch
Details | Diff | Splinter Review
1557233-dont-use-DOM-storge.patch
8.55 KB, patch
mkmelin
: review-
Details | Diff | Splinter Review
1557233-addEngineWithDetails.patch
1.45 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review

+++ This bug was initially created as a clone of Bug #1557018 +++

JavaScript error: chrome://messenger/content/newmailaccount/accountProvisioner.js, line 36: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMStorageManager.createStorage]

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=21d22b2f541258d3d1cf96c7ba5ad73e96&tochange=155a7e2117e575ff6de6caa3dfe5b076cb

That looks like bug 1555562, but there weren't changes to createStorage:
https://hg.mozilla.org/mozilla-central/rev/d39a023bdbb63114e2824c3d4d46a81c2c05bbd6

For the record, we have two of these calls in the system:
mail/components/newmailaccount/content/accountProvisioner.js
36 return Services.domStorageManager.createStorage(null, principal, principal, url);

mail/test/mozmill/newmailaccount/test-newmailaccount.js
1208 let storage = dsm.createStorage(null, principal, principal, url);

Ehsan, do we need to make any adjustment there?

I noticed that M-C code passes an empty string as last parameter:
https://searchfox.org/mozilla-central/search?q=.createStorage(&case=false&regexp=false&path=

but even with that change, tests don't pass.

Flags: needinfo?(ehsan)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/41e1b638798e
temporarily disable test-newmailaccount.js. rs=bustage-fix DONTBUILD

The last argument didn't change, you can keep passing an empty string, but now you should pass your principals twice! Like:

  let storage = gLocSvc.domstoremgr.createStorage(null, principal, principal, "");
Flags: needinfo?(ehsan)

Ah I see, searchfox was lagging. I landed bug 1556812 today which fixed something related. Bug 1557122 is also related. If this isn't fixed by any of these, you're gonna have to debug the tests to see what's going wrong I'm afraid. :-)

Flags: needinfo?(ehsan)
Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test]

Now we see this failure on comm-beta:
https://treeherder.mozilla.org/#/jobs?repo=comm-beta&revision=f63b58f41e227052e0614300bd642c688f0b3429

I've just moved the pinning forward from FF beta 9 to FF beta 11. So our beta will now be broken with
JavaScript error: chrome://messenger/content/newmailaccount/accountProvisioner.js, line 36: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMStorageManager.createStorage]

Note that none of bug 1555562, bug 1556812 or bug 1557122 were uplifted to beta.

Ben, could you look at this, seems like account creation is broken now on trunk and beta.

Flags: needinfo?(ben.bucksch)

Here's a try run with M-B pinned to 4c434d19d03d5461e54fa22dfb82eaed4cd6631b which is FF 68 beta 10, that will narrow it down.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d74f99a47c34231a4ea7b8f1e6ebfd158efba58d

Found it: Bug 1534431. That is in the intersection of the two ranges, got uplifted to FF beta between 9 and 10. My theory will be proven by the try run which is pinned to beta 10 and will also fail.

Flags: needinfo?(ben.bucksch)

Jan and Andrew: What do we need to do to get out tests going again?

We use createStorage() twice:
https://searchfox.org/comm-central/search?q=.createStorage%28&path=mail%2F

We need to provide a "client ID" now? How do we do that?

Flags: needinfo?(jvarga)
Flags: needinfo?(bugmail)
Regressed by: 1534431

How about setting Services.prefs.setBoolPref("dom.storage.client_validation", false); for TB?

Yeah, just set the pref.

Flags: needinfo?(jvarga)

Thanks, Jan. I'll deal with trunk when I'm off mobile internet again. I already messed up while on a moving train :-(

TB 68 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/facade754d64cd91020475f9d7b66fbf7c0aa652
https://hg.mozilla.org/releases/comm-beta/rev/b22f400e1734a9ad5f058cd750abab2fb7aa75f2

Flags: needinfo?(bugmail)

(In reply to Jan Varga [:janv] from comment #13)

Yeah, just set the pref.

OK, setting the pref globally was a knee-jerk reaction which was certainly valid to fix the tests. Thinking about it a bit more, I'd like to understand what's going on. The failing tests are not Xpcshell tests which in the case of FF set the preference, the failing test is a MozMill test. I don't know whether you remember MozMIll from FF from the olden days, but it's a test framework that runs a full TB session very much like a user staring a session. So what's the story about the client ID and why does TB not provide one? And should it?

Flags: needinfo?(jvarga)
Flags: needinfo?(bugmail)
Keywords: leave-open
Target Milestone: --- → Thunderbird 69.0

The client validation is a new security feature in new local storage implementation. It uses the Clients API to verify that the content process which asks for local storage data has actually a DOM window for that.
So if you don't pass a window to nsIDOMStorageManager.createStorage, we can't get a client ID, because a window is needed for that.
There are obviously no DOM windows in xpcshell, so the only way how to make it work is to disable client validation.

Flags: needinfo?(jvarga)
Attached patch set the pref — — Splinter Review

OK, this is the simple patch to set the pref and that worked on beta.

However, on trunk, after backing out rev 41e1b638798e, 27 of the 29 test still fail. This time with
EXCEPTION: Expected value of expression is not 'true'
EXCEPTION: Timeout waiting for modal dialog to open.

So it looks like some other stuff has crept in here while the tests were disabled :-(

I just saw comment #16 come in, so perhaps instead of setting the pref, we can pass a window on the first parameter. We'll see when we get the test going in general.

Can I leave this to the night shift?

Flags: needinfo?(bugmail) → needinfo?(geoff)

Passing a window can work too, I would need to know that code better to be sure.

Flags: needinfo?(geoff)

Not sure why the NI was removed :-( - It was for comment #17, that is, the test still fails even if the pref is set (which works on beta).

Anyway, I'll try to see whether passing a windows is a replacement of setting the pref, but it will still fail due to some other reasons.

The night shift is going home now, but I'll put the NI again. Maybe I can also get Aceman interested.

Flags: needinfo?(geoff)
Flags: needinfo?(acelists)
Attached patch 1557233-pass-window.patch (obsolete) — — Splinter Review

Passing a window makes 11 tests pass, 18 fail, I see:

JavaScript error: chrome://messenger/content/newmailaccount/accountProvisioner.js, line 282: NS_ERROR_FAILURE:
[10732, Main Thread] WARNING: NS_ENSURE_TRUE(aURI) failed: file c:/mozilla-source/comm-central/netwerk/dns/nsEffectiveTLDService.cpp, line 134
[10732, Main Thread] WARNING: No active window: file c:/mozilla-source/comm-central/js/xpconnect/src/XPCJSContext.cpp, line 662

and awful lot. Line 282 is let name = EmailAccountProvisioner.storage.getItem("name") ...

I'll try again with setting the pref to see what errors I can see there.

Assignee: nobody → jorgk

OK, just setting the pref like we did successfully on beta still gives:
[10720, Main Thread] WARNING: NS_ENSURE_TRUE(aURI) failed: file c:/mozilla-source/comm-central/netwerk/dns/nsEffectiveTLDService.cpp, line 134
[10720, Main Thread] WARNING: No active window: file c:/mozilla-source/comm-central/js/xpconnect/src/XPCJSContext.cpp, line 662
but not the JS error in accountProvisioner.js, line 282.

2 test pass, 27 fail. So is appears that all those failures are still related to the storage manager and that the pref doesn't help on trunk. Passing the window makes things slightly better.

Assignee: jorgk → nobody

Back to passing the window, I guess these errors are more relevant:
[10388, Main Thread] WARNING: 'NS_FAILED(rv)', file c:/mozilla-source/comm-central/toolkit/components/antitracking/StorageAccess.cpp, line 30
[10388, IPDL Background] WARNING: '!svc->HasWindow(aContentParentId, aPrincipalInfo, aClientId.ref())', file c:/mozilla-source/comm-central/dom/localstorage/ActorsParent.cpp, line 3220
[10388, IPDL Background] WARNING: '!VerifyClientId(mContentParentId, commonParams.principalInfo(), params.clientId())', file c:/mozilla-source/comm-central/dom/localstorage/ActorsParent.cpp, line 6431
[10388, IPDL Background] WARNING: '!VerifyRequestParams()', file c:/mozilla-source/comm-central/dom/localstorage/ActorsParent.cpp, line 6483
[10388, IPDL Background] WARNING: 'NS_FAILED(rv)', file c:/mozilla-source/comm-central/dom/localstorage/ActorsParent.cpp, line 6605
[10388, Main Thread] WARNING: 'NS_FAILED(rv)', file c:/mozilla-source/comm-central/dom/localstorage/LSObject.cpp, line 876
[10388, Main Thread] WARNING: 'NS_FAILED(rv)', file c:/mozilla-source/comm-central/dom/localstorage/LSObject.cpp, line 576
JavaScript error: chrome://messenger/content/newmailaccount/accountProvisioner.js, line 282: NS_ERROR_FAILURE:

ActorsParent.cpp, line 3220 is inside VerifyClientId(), so I guess when passing the window, we're triggering that code.

I'm not sure which window gets passed in, this needs more experimenting.

Attached patch 1557233-pass-window.patch (obsolete) — — Splinter Review

Here another variation passing the "official" main window, not some random account creation window which might happen to be on the screen. Same error.

So that might conclude the "pass the window" experiment and bring us back the question why that pref doesn't work on trunk any more.

Flags: needinfo?(geoff)
Flags: needinfo?(acelists)

Jan, can you offer some further advice here? When setting the pref worked on beta and didn't work on trunk, I thought some other issue has crept in here and all the 27 subtest fail for some other reason. Doing the experiment with passing the window in has the result that only 18 tests fail. So somehow I have the impression that tests failures are still related to the storage issue.

Has anything changed in the 69 cycle that could have an impact here? Too bad we didn't set the pref straight away when the issue started, maybe we would have found a change of behaviour in the meantime.

For the record: The issue first started with M-C at 155a7e2117e575ff6de6caa3dfe5b076cb and corresponding C-C at 4a37dadf6819 on this push:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=4a37dadf68195123ad4e38b39487a64c15f28d18

Here's a try based on those two versions with the pref set to see whether the pref worked on 69 code back then:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e8877089321d77d4430c034500aafaf19d7617d8

Flags: needinfo?(jvarga)

OK, tests pass in that combination, so I'll do some bisecting to see when the pref lost its power.

So the regression range is 15 days:

5th June: M-C 155a7e2117e575ff6de6caa3dfe5b076cb, C-C 4a37dadf6819 - pref working
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e8877089321d77d4430c034500aafaf19d7617d8

10th June: M-C 7a44faddc33d2e5d7435fbff216cc022d0, C-C 222be3dc3eb1 - pref working, tests passed
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5ad24fa3e7fab2eb99b181491f1d5245c7b06026

15th June: M-C b3a69dff7e25f1c1107520c238dd282405, C-C 793a15b6f65e - tests failed
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=74c3e83895de843a4b169999d2676182503a29c2

20th June: local build, pref not effective

Sadly I can't work out that the test failures means for manual operation, hence the expensive and slow try runs.

Here's another idea: Stop using that DOM storage for one string, the user name, be be carried around between invocations of the panel. Why can't we use a pref for that, like mail.newmailaccount.username or some such? With this patch, just removing the storage, 27 tests fails, same as if I were to set the pref at current tip.

Assignee: nobody → jorgk

Another variation, using a pref instead of DOM storage. 27 tests fail. So I come to think that the pref is still working but something else broke. Only strange that more tests pass when passing the window to the DOM storage creation.

Let's bisect some more between 10th and 15th of June:

10th June: M-C 7a44faddc33d2e5d7435fbff216cc022d0, C-C 222be3dc3eb1 - pref working, tests passed
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5ad24fa3e7fab2eb99b181491f1d5245c7b06026

11th June: 0679bf09303e001b724bb4852237277697 - 02a09c7cc1e3
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=89698c590fa00cfb67a6de49b426f33d3c209196

12th June: 43b2d57b25cc16d0571e6bef6a414abe24 - d350f0aa49d5
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=db1a292bf43510661c5c28dc4ec2401cc6bbc6bb

13th June: 4a63f0a3a1f26e2a377ffbd477ba050e16 - c358a56e279b
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ca68309acdad39b1aed7c27b3a0a0df01840cc8e

14th June: 3a8238bf97de2eb4bd73d05512242122d8 - 99b37b22c1f1
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4484f9355e63a01210043c8b84ac82fca6d2ad37

15th June: M-C b3a69dff7e25f1c1107520c238dd282405, C-C 793a15b6f65e - tests failed
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=74c3e83895de843a4b169999d2676182503a29c2

d72ab884f0394d6e9ab723f03bd43c90a8 - 3e76c29bfb96 worked, 7445a65187306dfa0fc18492e7b198887a - ae1624177e2c failed, so

C-C: https://hg.mozilla.org/comm-central/pushloghtml?fromchange=3e76c29bfb96&tochange=ae1624177e2c - no change here, only a backout
M-C: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d72ab884f0394d6e9ab723f03bd43c90a8&tochange=7445a65187306dfa0fc18492e7b198887a

So whatever landed in that M-C range broke the tests further so that setting the pref doesn't fix them any more. Mystery.

EDIT, I ran more tries:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=97447fa039dc9f8969f566725e7f2e54847bca68 - worked
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d662ce70053a9689c0b8f8bae63ad23bcc9ad028 - failed

So we're down to:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=37e68e5fbb18940c4e8636f91dfd3071d5a6d7aa&tochange=ec8bf3fa989d552b6730b1fcad10ecad305e0769

Sadly we're now hit by bug 1466381 and the range doesn't display.

After more tries
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ded8a2ec8f0b0ce8532c0a372af28eb11aa61b6c - worked
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1e9ddc0e970972ed3c2b0a6d8e2fba0cb89be51e - failed

the range is down to this:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bc46a287b8988d259c0b926b8789af9e3b2d9b35&tochange=bc71dba721d7cee0d78356aef41125dd41af45f2

Here's that range, and of course bc46a287b8988d259c0b926b8789af9e3b2d9b35 still works.

bc71dba721d7cee0d78356aef41125dd41af45f2	Brindusan Cristian — Backed out changeset 64b2f8306217 (bug 1536871) for dt failures at browser_fontinspector.js. CLOSED TREE
a7c18cfb94b3846a25a130c60e45552dc9884a72	alwu — Bug 1556079 - part1 : hide cue if we can't find a good place to put it fully inside the rendering area. r=heycam
f382cafbbebc73e9ebae6f99b6b281475d3e86d9	Thomas Nguyen — Bug 1557636 Use Referrer info in worker and fix sharedworker's wrong referrer r=baku
3e148406af28449ad570292b1892ac924f269ab4	Ehsan Akhgari — Bug 1559184 - Fix the damage caused by clang-format to DOMMatrix.h; r=bzbarsky
45ab583f7acb7367e9eac175807b9244876854e7	Karan Sapolia — Bug 1553237 - Simplify nsISearchService.addEngineWithDetails to one signature. r=Standard8
65393203d1c5615f2894e8fbdf6e437688293ca1	Alastor Wu — Bug 1557882 - part2 : add wpt 'start_alignment.html'. r=heycam
50efff6cae914849fff8ee9fbf6dc2d153210839	Alastor Wu — Bug 1557882 - part1 : delay setting 'unicode-bidi' until finishing the position calculation. r=heycam
70fa4fef28c6f109bcc643537521f20fbd7bfd29	Yury Delendik — Bug 1555518 - Fixes original stack display for wasm files, and ref to original source. r=jlast
0cb2bef9b2518cc985ae6e0850612ba1e646fe79	Marco Bonardo — * Bug 1557302 - Enter on a fully typed @alias should move the caret at the end. r=adw
72e19189bb99e159b2974cbcda69647211962df3	Brendan Dahl — Bug 1552049 - Enable XUL persistence for all system privilege docs. r=smaug
bc46a287b8988d259c0b926b8789af9e3b2d9b35	Andrea Marchesini — Bug 1559109 - Improve comments about StructuredCloneTags, r=bzbarsky

It's not bug 1552049 since I have backed this out locally.

So I think the storage guys are off the hook well and truly. So over to the night shift again:

Geoff, if you have time, apply the pref patch "set the pref" and then try of figure out what change in the range given above causes these tests to fail. What we learned from this: Never leave a test switched off for too long, you might not notice that next thing that breaks it further.

Giving this range a hard look, I can only see bug 1557636 which may be related. Fixing comments (bug 1559109) and white-space issues (bug 1559184) surely won't cause this, and everything else looks really unrelated.

Flags: needinfo?(jvarga) → needinfo?(geoff)

OK, I backed out f382cafbbebc73e9ebae6f99b6b281475d3e86d9 - Thomas Nguyen - Bug 1557636, didn't fix it. I give up.

Grrrr, I finally found it:
45ab583f7acb7367e9eac175807b9244876854e7 - Karan Sapolia — Bug 1553237 - Simplify nsISearchService.addEngineWithDetails to one signature. r=Standard8

And we use that here:
https://searchfox.org/comm-central/rev/7ced9f2f93c1c8c75e820764f631f58fcabfdcef/mail/test/mozmill/newmailaccount/test-newmailaccount.js#68

Flags: needinfo?(geoff)

I have to stop, this still doesn't work. Geoff, can you take a look after all.

Also, we're busted big time :-(

Flags: needinfo?(geoff)

I got this from Geoff via IRC. It works. I'll land it with the pref patch or the window patch.

Attachment #9073008 - Attachment is obsolete: true
Attachment #9073011 - Attachment is obsolete: true
Attachment #9073117 - Attachment is obsolete: true
Flags: needinfo?(geoff)
Attachment #9073130 - Flags: review+
Comment on attachment 9072980 [details] [diff] [review]
1557233-pass-window.patch

Doesn't work.
Attachment #9072980 - Attachment is obsolete: true
Attachment #9072983 - Attachment is obsolete: true
Comment on attachment 9073011 [details] [diff] [review]
1557233-dont-use-DOM-storge.patch

So this gets rid of using DOM storage and uses a simple pref instead. The tests pass, of course with Geoff's patch that fixes the search engine.

I have the feeling that DOM is going to introduce more and more security checks and I don't think it's worth to carry a simple string around. Certainly it's cool to use, but at a cost. This is the second bustage we're fixing here.
Attachment #9073011 - Attachment is obsolete: false
Attachment #9073011 - Flags: review?(geoff)
Attachment #9073011 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9073011 [details] [diff] [review]
1557233-dont-use-DOM-storge.patch

Review of attachment 9073011 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think we should use a pref for  this. It's valuable to know that dom storage works in tb. But also it's ugly to save some page internal state as a pref. I wonder if it's actually needed to be stored. Aren't we on the same page all the time (maybe?), so you could just have a page global variable?
Attachment #9073011 - Flags: review?(mkmelin+mozilla)
Attachment #9073011 - Flags: review?(geoff)
Attachment #9073011 - Flags: review-
Blocks: 1560485

(In reply to Magnus Melin [:mkmelin] from comment #38)

It's valuable to know that dom storage works in tb.
In fact, it does not work, otherwise we wouldn't be here. I have to pref off half the features, and it keeps breaking due to API changes, see bug 1389673, bug 1515992 and bug 1551533. As I said, it's a giant hack and overkill to store a single string against a fake URL "https://accountprovisioner.thunderbird.invalid". We don't use it anywhere else in the system, so I don't know what you gain from that knowledge.

But also it's ugly to save some page internal state as a pref. I wonder if it's actually needed to be stored. Aren't we on the same
page all the time (maybe?), so you could just have a page global variable?
What internal state? It's the user name the user entered, and that they want to move to the next panel, even when restarted. That's what prefs are for.

But as you please, I'll move it to bug 1560485.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/fb22bd6b99a7
Disable client ID checking when creating storage. r=me
https://hg.mozilla.org/comm-central/rev/a0c3fe6d7d95
Port bug 1553237: addEngineWithDetails() now takes parameters in an object and is async. r=jorgk
https://hg.mozilla.org/comm-central/rev/61d97e89fc63
Backed out changeset 41e1b638798e to re-enable tests. a=backout

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Keywords: regression
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: