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

Status: VERIFIED FIXED

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

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: 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.

Comment 1

[Tim Huang[:timhuang]]

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?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That sounds possible; I pressed Ctrl+Shift+C to open the Inspector tab and then switched to the Network tab.

Comment 3

[Tim Huang[:timhuang]]

Attachment: Part 1 : Make the devtools uses nodePrincipal instead of the system principal when fetching stylesheets.

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.

Comment 4

[Tim Huang[:timhuang]]

Attachment: Part 2 : Add a test cast to test that style editor can load stylesheets correctly with containers.

Comment 5

[James Andreou]

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.

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

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'

Comment 7

[Tanvi Vyas[:tanvi]]

(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.

Comment 8

[Tanvi Vyas[:tanvi]]

(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.

Comment 9

[Tim Huang[:timhuang]]

Attachment: Part 1 : Make the devtools uses nodePrincipal instead of the system principal when fetching stylesheets.

Rebase to m-c and update commit message.

Comment 10

[Tim Huang[:timhuang]]

Attachment: Part 2 : Add a test cast to test that style editor can load stylesheets correctly with containers.

Address the review comments, thanks for your review gl. Rebase to m-c and update commit message.

Comment 11

[Tim Huang[:timhuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f7409d9a4f6

Comment 12

[Tim Huang[:timhuang]]

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.

Comment 13

[Tim Huang[:timhuang]]

(In reply to Tim Huang[:timhuang] from comment #12)
> There are some test cases [1][2] fail due to this change.

[1] http://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/test/browser_rules_user-property-reset.js
[2] http://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/test/browser_rules_user-agent-styles-uneditable.js

Comment 14

[Tanvi Vyas[:tanvi]]

(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?

Comment 15

[Tim Huang[:timhuang]]

Attachment: Part 1 : Make the devtools uses nodePrincipal instead of the system principal when fetching stylesheets.

Modifying as comment 12 said.

Comment 16

[Gabriel Luong [:gl] (ΦωΦ)]

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.

Comment 17

[Tim Huang[:timhuang]]

Attachment: Part 1 : Make the devtools uses nodePrincipal instead of the system principal when fetching stylesheets.

Addressing the comment 16. Thanks, gl.

Comment 18

[Tim Huang[:timhuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=958b97db9ef4

Comment 19

[Tim Huang[:timhuang]]

Try looks good.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Pulsebot]

Backout by [e-mail]:
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

Comment 22

[Tim Huang[:timhuang]]

Attachment: Part 2 : Add a test cast to test that style editor can load stylesheets correctly with containers.

Fixing the eslint failures.

Comment 23

[Tim Huang[:timhuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a93ff4200ca3

Comment 24

[Tim Huang[:timhuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cba040a1ecf4

Comment 25

[Tim Huang[:timhuang]]

Try looks fine.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
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

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f57d23de8ac2
https://hg.mozilla.org/mozilla-central/rev/1d657d984bed

Comment 28

[Tanvi Vyas[:tanvi]]

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

Comment 29

[Tanvi Vyas[:tanvi]]

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

Comment 30

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 31

[Kamil Jozwiak [:kjozwiak]]

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

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/72f41d45c4de
https://hg.mozilla.org/releases/mozilla-aurora/rev/9b0f979295b7

Comment 33

[Kamil Jozwiak [:kjozwiak]]

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.

Comment 34

[Gerry Chang [:gchang]]

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.