Closed Bug 1315092 Opened 8 years ago Closed 7 years ago

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

Categories

(Firefox :: Site Identity, defect)

Product:
Firefox
Component:
Site Identity
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 - fixed
firefox56 - fixed
firefox57 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: nika)

References

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell fixed:product])

Attachments

(3 files)

Correctly ignore mPrivateBrowsingId when constructing permission keys
1.51 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
BETA - Correctly ignore mPrivateBrowsingId when constructing permission keys
2.34 KB, patch
jcristau
: approval-mozilla-beta+
Details | Diff | Splinter Review
re-enable browser_identity_UI view-source tests
1.09 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review 
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)
(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)
Flags: needinfo?(michael)
(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.
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)
Flags: needinfo?(michael)
Assignee: nobody → michael
tracking-firefox55: --- → ?
tracking-firefox56: --- → ?
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-
(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 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+
Flags: needinfo?(ehsan)
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/094ea9f8029b
Correctly ignore mPrivateBrowsingId when constructing permission keys, r=ehsan
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
https://hg.mozilla.org/mozilla-central/rev/094ea9f8029b
https://hg.mozilla.org/mozilla-central/rev/01b0662430d9
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
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)
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 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+
Whiteboard: [stockwell fixed:product]
Looks like this still fails on autoland?  Is that the same bug?
Flags: needinfo?(michael)
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 → ---
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 -
Component: General → Site Identity and Permission Panels
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.
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.
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)
Flags: needinfo?(michael)
Attachment #8900911 - Flags: review?(ehsan) → review+
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59d0cfcfd4a7
re-enable browser_identity_UI view-source tests, r=ehsan
https://hg.mozilla.org/mozilla-central/rev/59d0cfcfd4a7
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: