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

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Site Identity and Permission Panels
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: Treeherder Bug Filer, Assigned: Nika)

Tracking

({intermittent-failure})

unspecified
Firefox 56
intermittent-failure
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55- fixed, firefox56- fixed, firefox57 fixed)

Details

(Whiteboard: [stockwell fixed:product])

Attachments

(3 attachments)

Comment 1

11 months ago
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

11 months ago
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.
Blocks: 1374665
Flags: needinfo?(michael)

Comment 3

11 months ago
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
(Assignee)

Comment 4

11 months ago
(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 :).
Flags: needinfo?(michael)
(Assignee)

Updated

11 months ago
Flags: needinfo?(michael)

Comment 5

11 months ago
(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.
(Assignee)

Comment 6

11 months ago
Created attachment 8883561 [details] [diff] [review]
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
Attachment #8883561 - Flags: review?(ehsan)
(Assignee)

Updated

11 months ago
Flags: needinfo?(michael)
(Assignee)

Updated

11 months ago
Assignee: nobody → michael
tracking-firefox55: --- → ?
tracking-firefox56: --- → ?

Comment 7

11 months ago
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.  :-)
Attachment #8883561 - Flags: review?(ehsan) → review-
(Assignee)

Comment 8

11 months ago
(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.
Flags: needinfo?(ehsan)

Comment 9

11 months ago
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.
Attachment #8883561 - Flags: review- → review+

Updated

11 months ago
Flags: needinfo?(ehsan)

Comment 10

11 months ago
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/094ea9f8029b
Correctly ignore mPrivateBrowsingId when constructing permission keys, r=ehsan

Comment 11

11 months ago
Pushed by michael@thelayzells.com:
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

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/094ea9f8029b
https://hg.mozilla.org/mozilla-central/rev/01b0662430d9
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56

Comment 13

11 months ago
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.
tracking-firefox55: ? → -
tracking-firefox56: ? → -
Flags: needinfo?(michael)
(Assignee)

Comment 14

11 months ago
Created attachment 8884023 [details] [diff] [review]
BETA - Correctly ignore mPrivateBrowsingId when constructing permission keys

MozReview-Commit-ID: 3uCvKE5MxR5
(Assignee)

Comment 15

11 months ago
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.
Flags: needinfo?(michael)
Attachment #8884023 - Flags: approval-mozilla-beta?

Comment 16

11 months ago
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 on attachment 8884023 [details] [diff] [review]
BETA - Correctly ignore mPrivateBrowsingId when constructing permission keys

permissions manager fix for private browsing mode, beta55+
Attachment #8884023 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
status-firefox54: --- → unaffected
status-firefox55: --- → affected
status-firefox-esr52: --- → unaffected

Comment 18

11 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/6c28e5e116cb
status-firefox55: affected → fixed
Whiteboard: [stockwell fixed:product]

Comment 19

10 months ago
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

10 months ago
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

10 months ago
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

10 months ago
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

9 months ago
Looks like this still fails on autoland?  Is that the same bug?
Flags: needinfo?(michael)
(Assignee)

Comment 24

9 months ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

9 months ago
Summary: Intermittent browser/base/content/test/general/browser_identity_UI.js | Test timed out - → Intermittent browser/base/content/test/siteIdentity/browser_identity_UI.js | Test timed out -

Updated

9 months ago
Duplicate of this bug: 1388598

Updated

9 months ago
Component: General → Site Identity and Permission Panels

Comment 26

9 months ago
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
(Assignee)

Comment 27

9 months ago
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

9 months ago
Hmm, note the *siteIdentity* in the URL of the test, not quite sure if these are the same, perhaps I have confused myself here?
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

9 months ago
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
(Assignee)

Comment 31

9 months ago
Created attachment 8900911 [details] [diff] [review]
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.
Attachment #8900911 - Flags: review?(ehsan)
(Assignee)

Updated

9 months ago
Flags: needinfo?(michael)

Updated

9 months ago
Attachment #8900911 - Flags: review?(ehsan) → review+

Comment 32

9 months ago
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59d0cfcfd4a7
re-enable browser_identity_UI view-source tests, r=ehsan

Comment 33

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/59d0cfcfd4a7
Status: REOPENED → RESOLVED
Last Resolved: 11 months ago9 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED

Comment 34

5 months ago
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
You need to log in before you can comment on or make changes to this bug.