1315092 - Intermittent browser/base/content/test/siteIdentity/browser_identity_UI.js | Test timed out -

Status: RESOLVED FIXED

(Firefox :: Site Identity, defect)

Description

[Treeherder Bug Filer]

Filed by: philringnalda [at] gmail.com

https://treeherder.mozilla.org/logviewer.html#?job_id=6105469&repo=autoland

https://queue.taskcluster.net/v1/task/VVVBRa3tTY69WpNfRvGmaw/runs/0/artifacts/public%2Flogs%2Flive_backing.log

Comment 1

[Intermittent Failures Robot]

5 failures in 892 pushes (0.006 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* mozilla-inbound: 4
* autoland: 1

Platform breakdown:
* osx-10-10: 5

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1315092&startday=2017-06-19&endday=2017-06-25&tree=all

Comment 2

[:Gijs (out for now; he/him)]

This intermittent was mostly dead before (cf: https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1315092&endday=2017-06-26&startday=2017-04-01&tree=trunk ), but now we're hitting:

07:05:33     INFO - GECKO(1714) | [Child 1716] WARNING: This content process hasn't received the permissions for http://example.com yet: file /home/worker/workspace/build/src/extensions/cookie/nsPermissionManager.cpp, line 3330
07:05:33     INFO - GECKO(1714) | Assertion failure: PermissionAvaliable(prin, aType), at /home/worker/workspace/build/src/extensions/cookie/nsPermissionManager.cpp:2273

(e.g. https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=108989831&lineNumber=3180 )

This assertion was added in bug 1362791, and bug 1374665 is the recent change in the window where this started occurring (and it being backed out and relanding maaaaaybe vaguely matches the orange count going back to 0 and then back up?). Michael, can you take a look? Especially wondering if we should hold off uplift of 1374665 if my conjecturing here (without reading patches or much context, so please correct me!) is anywhere near right.

Comment 3

[Intermittent Failures Robot]

40 failures in 718 pushes (0.056 failures/push) were associated with this bug in the last 7 days. 

This is the #40 most frequent failure this week.  

** This failure happened more than 30 times this week! Resolving this bug is a high priority. **

** Try to resolve this bug as soon as possible. If unresolved for 2 weeks, the affected test(s) may be disabled. ** 

Repository breakdown:
* autoland: 19
* mozilla-inbound: 14
* mozilla-central: 6
* try: 1

Platform breakdown:
* osx-10-10: 38
* linux32: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1315092&startday=2017-06-26&endday=2017-07-02&tree=all

Comment 4

[Nika Layzell [:nika] (ni? for response)]

(In reply to :Gijs from comment #2)
> This assertion was added in bug 1362791, and bug 1374665 is the recent
> change in the window where this started occurring (and it being backed out
> and relanding maaaaaybe vaguely matches the orange count going back to 0 and
> then back up?). Michael, can you take a look? Especially wondering if we
> should hold off uplift of 1374665 if my conjecturing here (without reading
> patches or much context, so please correct me!) is anywhere near right.

Sorry for taking so long to look at this. I'm guessing that they are likely to be related which is concerning to me as it points to the implementation of bug 1374665 being incorrect.

I would be very interested to know if this intermittent is appearing on beta now that that the patch has been uplifted (unfortunately before I looked at this), and I'll look into figuring out the cause of this asap :).

Comment 5

[:Gijs (out for now; he/him)]

(In reply to Michael Layzell [:mystor] from comment #4)
> I would be very interested to know if this intermittent is appearing on beta
> now that that the patch has been uplifted

Not so far (AFAICT), though it's hard to know if that's coincidence to do with the frequency of commits (and thus builds) on beta.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Attachment: Correctly ignore mPrivateBrowsingId when constructing permission keys

The problem which was causing this failure was that the new permission key code
path incorrectly didn't clear the mPrivateBrowsingId origin attribute field,
causing it to sometimes transmit the wrong set of permissions to the content
process. This should fix that problem.

This will need to be uplifted to beta.

MozReview-Commit-ID: 3uCvKE5MxR5

Comment 7

[(no longer active)]

Comment on attachment 8883561 [details] [diff] [review]
Correctly ignore mPrivateBrowsingId when constructing permission keys

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

OriginAttributes needs to be a responsible grown up class and initialize itself.  Not every code can go around and do it for it forever.  :-)

Comment 8

[Nika Layzell [:nika] (ni? for response)]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #7)
> Comment on attachment 8883561 [details] [diff] [review]
> Correctly ignore mPrivateBrowsingId when constructing permission keys
> 
> Review of attachment 8883561 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> OriginAttributes needs to be a responsible grown up class and initialize
> itself.  Not every code can go around and do it for it forever.  :-)

I think you misunderstand what this patch is fixing, this has nothing to do with OriginAttributes initializing itself.

The problem here is that we initialize the OriginAttributes from a suffix string, which we extract from the inputted origin string. If that origin string has a privateBrowsingId, our OriginAttributes now also does. However, nsPermissionManager is supposed to ignore the privatebrowsingid, so we have to explicitly modify our passed-in OriginAttributes to make sure that we don't look at it. This is done everywhere else in nsPermissionManager in our special functions for parsing nsIPrincipals to origins and vice-versa, but I forgot to do it on this codepath.

This patch is just changing the code to correctly ignore private browsing ID.

Comment 9

[(no longer active)]

Comment on attachment 8883561 [details] [diff] [review]
Correctly ignore mPrivateBrowsingId when constructing permission keys

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

Oh yes indeed I was misunderstanding this, I thought mPrivateBrowsingId is being left uninitialized here.  My apologies.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/094ea9f8029b
Correctly ignore mPrivateBrowsingId when constructing permission keys, r=ehsan

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/01b0662430d9
Part 2: Correctly clear mPrivateBrowsingId during the assertion as well on a CLOSED TREE, a=bustage

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/094ea9f8029b
https://hg.mozilla.org/mozilla-central/rev/01b0662430d9

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Mystor, could you please nominate this for uplift to beta55?

I don't feel the need to track this intermittent-failure for 56/55.

Comment 14

[Nika Layzell [:nika] (ni? for response)]

Attachment: BETA - Correctly ignore mPrivateBrowsingId when constructing permission keys

MozReview-Commit-ID: 3uCvKE5MxR5

Comment 15

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8884023 [details] [diff] [review]
BETA - Correctly ignore mPrivateBrowsingId when constructing permission keys

Approval Request Comment
[Feature/Bug causing the regression]: bug 1374665
[User impact if declined]: permission checks may not work correctly on some webpages in private browsing mode.
[Is this code covered by automated tests?]: There are no explicit tests, but the problem was found due to an intermittent.
[Has the fix been verified in Nightly?]: The code has landed in nightly.
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Straightforward change to obviously (in retrospect) incorrect code to make it correct and match other code in the same module.
[String changes made/needed]: None

(In reply to Ritu Kothari (:ritu) from comment #13)
> Hi Mystor, could you please nominate this for uplift to beta55?
> 
> I don't feel the need to track this intermittent-failure for 56/55.

My reasoning for the tracking is that this intermittent was the symptom of a more serious bug (permissions in private browsing mode may not be handled correctly under some circumstances). 

Requesting uplift.

Comment 16

[Intermittent Failures Robot]

24 failures in 656 pushes (0.037 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* mozilla-inbound: 11
* autoland: 11
* try: 1
* oak: 1

Platform breakdown:
* osx-10-10: 14
* osx-cross: 8
* linux64: 1
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1315092&startday=2017-07-03&endday=2017-07-09&tree=all

Comment 17

[Julien Cristau [:jcristau]]

Comment on attachment 8884023 [details] [diff] [review]
BETA - Correctly ignore mPrivateBrowsingId when constructing permission keys

permissions manager fix for private browsing mode, beta55+

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6c28e5e116cb

Comment 19

[Intermittent Failures Robot]

9 failures in 720 pushes (0.013 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* try: 3
* autoland: 3
* pine: 2
* mozilla-esr52: 1

Platform breakdown:
* osx-10-10: 5
* osx-cross: 2
* windows7-32-vm: 1
* linux64: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1315092&startday=2017-07-10&endday=2017-07-16&tree=all

Comment 20

[Intermittent Failures Robot]

4 failures in 822 pushes (0.005 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* try: 4

Platform breakdown:
* osx-10-10: 3
* linux64: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1315092&startday=2017-07-17&endday=2017-07-23&tree=all

Comment 21

[Intermittent Failures Robot]

3 failures in 1008 pushes (0.003 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* oak: 2
* try: 1

Platform breakdown:
* osx-10-10: 2
* linux64: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1315092&startday=2017-07-24&endday=2017-07-30&tree=all

Comment 22

[Intermittent Failures Robot]

7 failures in 888 pushes (0.008 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* autoland: 4
* try: 2
* oak: 1

Platform breakdown:
* osx-10-10: 6
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1315092&startday=2017-07-31&endday=2017-08-06&tree=all

Comment 23

[(no longer active)]

Looks like this still fails on autoland?  Is that the same bug?

Comment 24

[Nika Layzell [:nika] (ni? for response)]

Not sure if it's exactly the same bug, but it looks the same. I'll have to look into it again.

Keeping the needinfo pending so I take a shot at it later today.

Comment 26

[Intermittent Failures Robot]

15 failures in 901 pushes (0.017 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* mozilla-inbound: 14
* try: 1

Platform breakdown:
* osx-10-10: 14
* windows8-64: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1315092&startday=2017-08-07&endday=2017-08-13&tree=all

Comment 27

[Nika Layzell [:nika] (ni? for response)]

There's a chance that this is caused by bug 1379345, as we do try to load view-source URLs in that test, but I don't think we'd ever load them out of order... so.

I'm inclined to fix bug 1379345 and see if the rate drops to 0. If it doesn't, I'll come back to this one.

Comment 28

[(no longer active)]

Hmm, note the *siteIdentity* in the URL of the test, not quite sure if these are the same, perhaps I have confused myself here?

Comment 29

[Gabor Krizsanits (INACTIVE)]

This test is perma crashing locally on OSX on the "view-source:http://example.com/" part. With a quite unrelated patch it starts failing on windows 7 dbg as well on try, so I cannot land my patch for bug 1376895.

Maybe on try we run it only for windows? 

Since it's perma failing locally on osx with the very same error, and I'm pretty sure on windows dbg the only reason for this test not to fail, is because it's poorly written and calls a bunch of async BrowserTestUtils functions and then executing the next test without waiting for them. On windows debug, things are so slow that probably we navigate away for the next test before we would hit the crash.

I'm going to disable the view-source part since it's completely useless in the current state of the test, maybe after bug 1379345 it can be re-enabled again, but probably a complete rewrite will be needed.

Comment 30

[Intermittent Failures Robot]

18 failures in 949 pushes (0.019 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* mozilla-inbound: 13
* autoland: 3
* try: 1
* mozilla-central: 1

Platform breakdown:
* windows7-32: 4
* linux64: 4
* windows8-64: 3
* osx-10-10: 3
* linux32: 3
* macosx64-stylo: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1315092&startday=2017-08-14&endday=2017-08-20&tree=all

Comment 31

[Nika Layzell [:nika] (ni? for response)]

Attachment: re-enable browser_identity_UI view-source tests

I think that these test failures should stop now that bug 1379345 has been fixed.

This patch re-enables the disabled section of code.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59d0cfcfd4a7
re-enable browser_identity_UI view-source tests, r=ehsan

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/59d0cfcfd4a7

Comment 34

[Intermittent Failures Robot]

1 failures in 889 pushes (0.001 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* mozilla-esr52: 1

Platform breakdown:
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1315092&startday=2017-12-04&endday=2017-12-10&tree=all