Hit MOZ_CRASH(Accessing the Subject Principal without an AutoJSAPI on the stack is forbidden) at /dom/base/nsContentUtils.cpp:2756 when dragging a tab over another window's tabstrip

RESOLVED FIXED in Firefox 51

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: kjozwiak, Assigned: mystor)

Tracking

(4 keywords)

51 Branch
mozilla51
crash, dogfood, regression, topcrash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox50 unaffected, firefox51+ fixed)

Details

(crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Created attachment 8782073 [details]
crashStack.txt

The latest version of m-c will instantly crashes when you drag a non-e10s window over the tab bar area of an e10s window.

* I've attached the stack received while using m-c debug

Crash Reports:
* bp-e3b65e3d-f591-414d-93eb-4673b2160817
* bp-2d72c80b-865e-456e-a921-8df372160817
* bp-f26856b9-5b48-4086-9e0a-b31192160817

STR:

* launch the latest m-c
** build used: fx51.0a1, buildId: 20160817030202, changeset: fe895421dfbe
* Select File -> New non-e10s Window
* drag the non-e10s window over the tab area of the original e10s window that opened when launching FX
* m-c will instantly crash

I'll try attaching a mozregression report as well.
(Reporter)

Updated

2 years ago
Summary: Hit MOZ_CRASH(Accessing the Subject Principal without an AutoJSAPI on the stack is forbidden) at /Users/kjozwiak/projects/mozilla-central/dom/base/nsContentUtils.cpp:2756 → Hit MOZ_CRASH(Accessing the Subject Principal without an AutoJSAPI on the stack is forbidden) at /dom/base/nsContentUtils.cpp:2756
(Reporter)

Comment 1

2 years ago
I went through mozregression and it appears like bug#1293739 is the culprit. Jonathan, mind taking a look?

Results:

2:27.70 INFO: Last good revision: 14974aa2862f2dd9fac16cd78cd88a6208c951d6
2:27.70 INFO: First bad revision: 8c3529c5f60a1b292eaa981b07c27091040a04cc
2:27.70 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=14974aa2862f2dd9fac16cd78cd88a6208c951d6&tochange=8c3529c5f60a1b292eaa981b07c27091040a04cc

2:28.89 INFO: Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1293739
Flags: needinfo?(jyc)

Comment 2

2 years ago
Hi Kamil,

Interesting. Checking it out. Can't immediately see what could be wrong, because those two commits are just renaming with sed and then |hg mv|.
mozregression's "Looks like the following bug has the changes which introduced the regression" assignment here was much too presumptive -- there are still tons of builds in the pushlog from comment 1.

The latest commit in the log is indeed associated with bug 1293739, but it's a *backout* -- so bug 1293739 can't possibly have caused this. (And as jyc noted, 1293739 is just a type-renaming, so even if the latest commit were its landing instead of its backout, it'd be extremely unlikely to have been involved)
Flags: needinfo?(jyc)
> there are still tons of builds in the pushlog from comment 1.

Sorry, I misspoke; I meant to say: "there are tons of other commits / other bugs in the pushlog from comment 1"

You might have better luck at getting a tight regression range if you instruct mozregression to use inbound (or autoland, if that's possible?)  ("mozregression --repo mozilla-inbound ...")
Given that the crash report backtraces are in DataTransfer/DataTransferItemList code, I'd direct initial suspicion at the changesets from bug 1290688 (e.g. "Michael Layzell — Bug 1290688 - Part 2: Don't include the types of files in DataTransfer.types, as per the spec, r=baku)
Flags: needinfo?(michael)
mozregression (with --repo mozilla-inbound) gives me this much-narrower range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e91880668542e00140784bc57e747aece36480ae&tochange=0c17b64aac661b09afadecbb7dfbd44575f42868

Looks like this is something from one of Michael's changes.
(Reporter)

Comment 7

2 years ago
> You might have better luck at getting a tight regression range if you
> instruct mozregression to use inbound (or autoland, if that's possible?) 
> ("mozregression --repo mozilla-inbound ...")

Thanks for the help Daniel. It looks like I got the same thing when using mozilla-inbound. Apologies for the red flag Jonathan.

14:20.29 INFO: Last good revision: e91880668542e00140784bc57e747aece36480ae
14:20.29 INFO: First bad revision: 5150bc3f6d211ff0b7ba9c52c9bb5a97b793fec4
14:20.29 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e91880668542e00140784bc57e747aece36480ae&tochange=5150bc3f6d211ff0b7ba9c52c9bb5a97b793fec4

14:21.60 INFO: Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1290688
I think this happened for me even while dragging an e10s tab over an e10s window tab strip (that is, I have e10s enabled generally and did not disable it for any particular tabs, and I still got this).
(Same here -- I don't need to specifically use a non-e10s window to trigger this.)
Summary: Hit MOZ_CRASH(Accessing the Subject Principal without an AutoJSAPI on the stack is forbidden) at /dom/base/nsContentUtils.cpp:2756 → Hit MOZ_CRASH(Accessing the Subject Principal without an AutoJSAPI on the stack is forbidden) at /dom/base/nsContentUtils.cpp:2756 when dragging a tab over another window's tabstrip

Comment 10

2 years ago
ditto linux x86_64: bp-82d1b4ca-ef6d-4676-86db-67f7d2160817
OS: Mac OS X → All
Hardware: x86 → All
Keywords: crash, dogfood, regression
Blocks: 1290688
status-firefox50: --- → unaffected
[Tracking Requested - why for this release]: crash in common operation. (I get this every time I drag a tab.)

This looks like this is the top crash for today's Nightly.
tracking-firefox51: --- → ?
Keywords: topcrash
(Assignee)

Comment 12

2 years ago
It seems like this is caused by DataTransferItemList::Types() calling DataTransferItemList::Files() calling IndexedGetter() which currently performs the permissions checks from GetDataAtInternal from before the DataTransferItem{,List} patch.

I think the correct solution would be to make those checks happen when actually trying to get the data, rather than when getting the types, and then don't actually call Files() from Types(), and iterate manually.

I'll try to have a patch up for that before I go home today.
Flags: needinfo?(michael)
(Assignee)

Comment 13

2 years ago
Created attachment 8782183 [details] [diff] [review]
Make DataTransfer::Types() not take the Subject Principal

MozReview-Commit-ID: 4aeGjTejP3z
Attachment #8782183 - Flags: review?(amarchesini)

Comment 14

2 years ago
(In reply to Michael Layzell [:mystor] from comment #12)
> It seems like this is caused by DataTransferItemList::Types() calling
> DataTransferItemList::Files() calling IndexedGetter() which currently
> performs the permissions checks from GetDataAtInternal from before the
> DataTransferItem{,List} patch.
> 

That's broken then. dataTransfer.types should never result in retrieving the data as a side effect.
(Assignee)

Comment 15

2 years ago
I believe that this updated patch should avoid accidentally getting the data when calling types.

Comment 16

2 years ago
(In reply to Michael Layzell [:mystor] from comment #15)
> I believe that this updated patch should avoid accidentally getting the data
> when calling types.

A brief glance suggests that it calls InternalGetter, no?
Michael, since you've been providing a patch, assign this bug to you. Feel free to re-assign, thanks.
Assignee: nobody → michael
Comment on attachment 8782183 [details] [diff] [review]
Make DataTransfer::Types() not take the Subject Principal

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

::: dom/events/DataTransfer.cpp
@@ +337,5 @@
>  
> +  for (uint32_t i = 0; i < mItems->Length(); ++i) {
> +    ErrorResult rv;
> +    DataTransferItem* item = mItems->InternalGetter(i, nsContentUtils::SystemPrincipal(), &rv);
> +    if (item && !rv.Failed() && item->Kind() == DataTransferItem::KIND_FILE) {

_if_ this rv contains an error, you must call SuppressException(), or propagate the error.

::: dom/events/DataTransferItemList.cpp
@@ +110,5 @@
> +
> +DataTransferItem*
> +DataTransferItemList::InternalGetter(uint32_t aIndex, nsIPrincipal* aSubjectPrincipal, ErrorResult& aRv) const
> +{
> +  MOZ_ASSERT(aSubjectPrincipal);

It's called subjectPrincipal, but it can be system. I don't like it. Call it aPrincipal instead.

@@ +124,5 @@
>    // drop event, allow retrieving the data except in the case where the
>    // source of the drag is in a child frame of the caller. In that case,
>    // we only allow access to data of the same principal. During other events,
>    // only allow access to the data with the same principal.
> +  if (item->Principal() && mIsCrossDomainSubFrameDrop &&

instead passing a systemprincipal, can you just skip this test for types?
Attachment #8782183 - Flags: review?(amarchesini)
tracking-e10s: --- → ?
(Assignee)

Comment 19

2 years ago
Created attachment 8782430 [details] [diff] [review]
Check subject principal in Data(), instead of IndexedGetter

This changes the place where we check principals for whether a subject can access the Data for a DataTransferItem.

Previously, I decided to perform this check in IndexedGetter, which meant that a page could not touch a DataTransferItem unless it had a principal which could access it. Now this check is performed when accessing the data itself. This lines up better with where this check was performed before the introduction of DataTransferItemList and DataTransferItem (which was GetDataAtInternal), and also allows me to simplify some of that existing logic.

This makes accessing DataTransferItems cheaper, as the indexed getter no longer has to retreive the data from the OS eagerly, and means that we now only take the subject principal in reasonable locations, at JS code boundaries.
Attachment #8782430 - Flags: review?(amarchesini)
(Assignee)

Updated

2 years ago
Attachment #8782183 - Attachment is obsolete: true

Updated

2 years ago
tracking-e10s: ? → +
Comment on attachment 8782430 [details] [diff] [review]
Check subject principal in Data(), instead of IndexedGetter

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

Big simplification on the code. Good. Before landing it, test if DnD of tabs works, also using containers. Thanks!

::: dom/events/DataTransfer.cpp
@@ +337,5 @@
>  
> +  for (uint32_t i = 0; i < mItems->Length(); ++i) {
> +    ErrorResult rv;
> +    bool found = false;
> +    DataTransferItem* item = mItems->IndexedGetter(i, found, rv);

if (!found || rv.Failed() || item->Kind() != DataTrasferItem::KIND_FILE) {
  rv.SuppressException();
  continue;
}

...

::: dom/events/DataTransferItem.cpp
@@ +280,5 @@
>      return nullptr;
>    }
>  
>    nsCOMPtr<nsIGlobalObject> global;
> +  RefPtr<DataTransfer> dataTransfer = mDataTransfer;

do you need to use this?

::: dom/events/DataTransferItemList.h
@@ +27,2 @@
>      , mIsExternal(aIsExternal)
>    {

MOZ_ASSERT(aDataTransfer);
Attachment #8782430 - Flags: review?(amarchesini) → review+

Comment 21

2 years ago
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c463cee57e3
Check subject principal in Data(), instead of IndexedGetter, r=baku

Updated

2 years ago
Duplicate of this bug: 1296499

Comment 23

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1c463cee57e3
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Tracking 51+ for this top crash.
tracking-firefox51: ? → +
(Assignee)

Updated

2 years ago
See Also: → bug 1297393

Updated

2 years ago
Depends on: 1297186
Crash volume for signature 'nsContentUtils::SubjectPrincipal':
 - nightly (version 51): 3022 crashes from 2016-08-01.
 - aurora  (version 50): 0 crashes from 2016-08-01.
 - beta    (version 49): 7 crashes from 2016-08-02.
 - release (version 48): 5 crashes from 2016-07-25.
 - esr     (version 45): 6 crashes from 2016-05-02.

Crash volume on the last weeks (Week N is from 08-22 to 08-28):
            W. N-1  W. N-2  W. N-3
 - nightly    2792       0       0
 - aurora        0       0       0
 - beta          5       2       0
 - release       1       1       1
 - esr           0       0       1

Affected platforms: Windows, Mac OS X, Linux

Crash rank on the last 7 days:
           Browser   Content     Plugin
 - nightly #1        #7
 - aurora
 - beta    #4336     #3106
 - release #5215
 - esr
status-firefox48: --- → affected
status-firefox49: --- → affected
status-firefox-esr45: --- → affected
status-firefox48: affected → ---
status-firefox49: affected → ---
status-firefox-esr45: affected → ---
Depends on: 1330979
You need to log in before you can comment on or make changes to this bug.