Closed Bug 1282660 Opened 4 years ago Closed 4 years ago

opening network devtools panel with container tab: fatal Assertion failure: originAttrsLoadInfo.mUserContextId == originAttrsLoadContext.mUserContextId, in NS_CompareLoadInfoAndLoadContext

Categories

(Core :: DOM: Security, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla50
Tracking Status
firefox49 --- verified
firefox50 --- verified

People

(Reporter: dbaron, Assigned: timhuang)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, crash, Whiteboard: [domsecurity-active][userContextId][OA][uplift49+])

Attachments

(3 files, 5 obsolete files)

32.44 KB, text/plain
Details
1.49 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
3.38 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
Attached file gdb session
I had a slack instance open in a container tab, and the throbber wouldn't stop spinning so I opened the network devtools tab to see what was going on.  This caused a crash due to a fatal assertion:

Assertion failure: originAttrsLoadInfo.mUserContextId == originAttrsLoadContext.mUserContextId (The value of mUserContextId in the loadContext and in the loadInfo are not the same!), at /home/dbaron/builds/ssd/mozilla-central/mozilla/netwerk/base/nsNetUtil.cpp:2441 in NS_CompareLoadInfoAndLoadContext

A gdb session is attached.
Assignee: nobody → tihuang
Status: NEW → ASSIGNED
Whiteboard: [domsecurity-active]
I can only reproduce the same issue on the inspector tab and style editor tab, but not on the network tab.

Dbaron, could you help on verifying this?
That sounds possible; I pressed Ctrl+Shift+C to open the Inspector tab and then switched to the Network tab.
The reason of this assertion is that the devtools uses the system principal to fetch stylesheets with the window's loadGroup. The loadContext from the loadGroup contains the window's userContextId which is different from the userContextId of the system principal, which causes this problem. Here, I make the fetch uses the right principal to solve this problem.
Attachment #8766202 - Flags: review?(gl)
I have a similar issue with the privateBrowsingId attribute. Similarly the system principal is being used by DevTools to create the LoadInfo. However, the system principal does not have private browsing set. In my patch for Bug 1281224 I was able to get around this by creating a new principal for the case of private browsing and not using the system principal. Maybe we can come up with a less hacky solution that works for the other attributes as well.
Priority: -- → P1
Whiteboard: [domsecurity-active] → [domsecurity-active][userContextId][OA]
Attachment #8766202 - Flags: review?(gl) → review+
Comment on attachment 8766203 [details] [diff] [review]
Part 2 : Add a test cast to test that style editor can load stylesheets correctly with containers.

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

::: devtools/client/styleeditor/test/browser_styleeditor_loading_with_containers.js
@@ +3,5 @@
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Bug 1282660 - Test that the stylesheets can be loaded correctly with containers.

We don't typically need to include the Bug # in the test file comment. So, please remove "Bug 1282660".

@@ +31,5 @@
> +  checkSheet(ui.editors[1], EXPECTED_SHEETS[1]);
> +});
> +
> +function* openTabInUserContext(uri, userContextId) {
> +  // open the tab in the correct userContextId

Capitalize 'Open'

@@ +34,5 @@
> +function* openTabInUserContext(uri, userContextId) {
> +  // open the tab in the correct userContextId
> +  let tab = gBrowser.addTab(uri, {userContextId});
> +
> +  // select tab and make sure its browser is focused

Captialize 'Select'
Attachment #8766203 - Flags: review?(gl) → review+
(In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #6)
> devtools/client/styleeditor/test/browser_styleeditor_loading_with_containers.
> js
> @@ +3,5 @@
> > +   http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +"use strict";
> > +
> > +// Bug 1282660 - Test that the stylesheets can be loaded correctly with containers.
> 
> We don't typically need to include the Bug # in the test file comment. So,
> please remove "Bug 1282660".
> 

I disagree.  Having the bug number in the test file helps when debugging test failures since you have a direct pointer to the bug related to the test.
(In reply to James Andreou from comment #5)
> I have a similar issue with the privateBrowsingId attribute. Similarly the
> system principal is being used by DevTools to create the LoadInfo. However,
> the system principal does not have private browsing set. In my patch for Bug
> 1281224 I was able to get around this by creating a new principal for the
> case of private browsing and not using the system principal. Maybe we can
> come up with a less hacky solution that works for the other attributes as
> well.

Won't the solution here work?  Since we are now passing the nodePrincipal.  I agree that in general we have a problem when we use systemPrincipal, since the real OriginAttributes are then lost.  Maybe we can put them in the loadInfo and when we see systemPrincipal, use the loadInfo to get the real OriginAttributes?  This is a bigger problem than this bug though, and probably needs a bug of its own.
Rebase to m-c and update commit message.
Attachment #8767015 - Flags: review+
Attachment #8766202 - Attachment is obsolete: true
Address the review comments, thanks for your review gl. Rebase to m-c and update commit message.
Attachment #8767017 - Flags: review+
Attachment #8766203 - Attachment is obsolete: true
There are some test cases [1][2] fail due to this change. The reason is that these test cases load default internal stylesheets from the URI: resource://gre-resources/*.css with the content principal, but these stylesheets can only be loaded by system principal.

Could we treat this 'resource:' protocol differently that we still use the system principal for 'resource:' protocol and strip the loadGroup for preventing this assertion.
Flags: needinfo?(tanvi)
Flags: needinfo?(gl)
(In reply to Tim Huang[:timhuang] from comment #12)
> There are some test cases [1][2] fail due to this change. The reason is that
> these test cases load default internal stylesheets from the URI:
> resource://gre-resources/*.css with the content principal, but these
> stylesheets can only be loaded by system principal.
> 
> Could we treat this 'resource:' protocol differently that we still use the
> system principal for 'resource:' protocol and strip the loadGroup for
> preventing this assertion.

Yes, lets try it.  gl, what do you think?
Flags: needinfo?(tanvi)
Attachment #8767015 - Attachment is obsolete: true
Comment on attachment 8768672 [details] [diff] [review]
Part 1 : Make the devtools uses nodePrincipal instead of the system principal when fetching stylesheets.

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

::: devtools/server/actors/stylesheets.js
@@ +448,5 @@
> +    // The default internal stylesheets load from the 'resource:' URL.
> +    if (/^resource:\/\//.test(this.href)) {
> +      delete options.principal;
> +      delete options.window;
> +    }

I don't think we should be assign values to an object and subsequently deleting it. It would be better just to set the loadFromCache, policy and charSet properties into options, and follow it up with a check to add the window and principal properties. 

Please use the r+ freely after these changes.
Attachment #8768672 - Flags: review?(gl) → review+
Attachment #8768672 - Attachment is obsolete: true
Flags: needinfo?(gl)
Try looks good.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/79f9307c33ea
Part 1 : Make the devtools uses nodePrincipal instead of the system principal when fetching stylesheets. r=gl
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c0f7ffa8e3f
Part 2 : Add a test case to test that style editor can load stylesheets correctly with containers. r=gl
Keywords: checkin-needed
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/951f04b12188
Backed out changeset 7c0f7ffa8e3f 
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d3db763a5e3
Backed out changeset 79f9307c33ea for eslint failures
Attachment #8767017 - Attachment is obsolete: true
Try looks fine.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f57d23de8ac2
Part 1 : Make the devtools uses nodePrincipal instead of the system principal when fetching stylesheets. r=gl
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d657d984bed
Part 2 : Add a test cast to test that style editor can load stylesheets correctly with containers. r=gl
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f57d23de8ac2
https://hg.mozilla.org/mozilla-central/rev/1d657d984bed
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1287802
Depends on: 1287607
Whiteboard: [domsecurity-active][userContextId][OA] → [domsecurity-active][userContextId][OA][uplift49+]
Comment on attachment 8770467 [details] [diff] [review]
Part 1 : Make the devtools uses nodePrincipal instead of the system principal when fetching stylesheets.

Approval Request Comment
[Feature/regressing bug #]:Mismatch in OriginAttributes causes an assertion failure that will crash the browser in a Container tab
[User impact if declined]: Browser crash when using the network devtools tab.
[Describe test coverage new/current, TreeHerder]:Test included
[Risks and why]: Simple 3 line change
[String/UUID change made/needed]: None
Attachment #8770467 - Flags: approval-mozilla-aurora?
Comment on attachment 8770915 [details] [diff] [review]
Part 2 : Add a test cast to test that style editor can load stylesheets correctly with containers.

Approval Request Comment
[Feature/regressing bug #]: Mismatch in OriginAttributes causes an assertion failure that will crash the browser in a Container tab
[User impact if declined]: Browser crash when using the network devtools tab.
[Describe test coverage new/current, TreeHerder]: This is the test
[Risks and why]: None, this is a test
[String/UUID change made/needed]: None
Attachment #8770915 - Flags: approval-mozilla-aurora?
Comment on attachment 8770467 [details] [diff] [review]
Part 1 : Make the devtools uses nodePrincipal instead of the system principal when fetching stylesheets.

Needed for the Test Pilot container/identity feature, taking it.
Attachment #8770467 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify+
Reproduced the original issue using the STR from comment #0 using the following build:
* fx50.0a1, buildId: 20160727153412, changeset: 0e3f8401b804 (using --enable-debug)
** received the following assertion: https://pastebin.mozilla.org/8887383

Went through verification using the following builds:
* fx50.0a1, buildId: 20160727153412, changeset: 0e3f8401b804 (using --enable-debug)

Used the following Test Cases:

* went through the original STR from comment#0 using all four containers types (Personal, Work, Banking, Shopping)
** also went through the above test case using PB mode
Went through verifiation using the following build:
* fx49.0a2, buildId, 20160728110524, changeset: fbf0cb3a7651

Went through the same test cases/steps outlined in comment#31 without any issues.
Status: RESOLVED → VERIFIED
Comment on attachment 8770915 [details] [diff] [review]
Part 2 : Add a test cast to test that style editor can load stylesheets correctly with containers.

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

Add a test for the patch. Take it in aurora.
Attachment #8770915 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1291321
Depends on: 1306892
No longer depends on: 1306892
You need to log in before you can comment on or make changes to this bug.