Closed Bug 1403024 Opened 7 years ago Closed 7 years ago

stylo: Add some diagnosis information to crash report when crash for adopting element across style backend

Categories

(Core :: CSS Parsing and Computation, defect, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(2 files)

Bug 1403024 part 1 - Add nsContentUtils::SchemeIs helper function.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1403024 part 2 - Add more diagnosis information before crashing for adopting element across style backend.
59 bytes, text/x-review-board-request
francois
: review+
Sylvestre
: approval-mozilla-beta+
Details 
This bug is for adding diagnosis information for bug 1402589.

See bug 1402589 comment 2 and discussion in http://logs.glob.uno/?c=mozilla%23servo#c757815
Comment on attachment 8912062 [details]
Bug 1403024 part 2 - Add more diagnosis information before crashing for adopting element across style backend.

f? for data collection review.

After enabling stylo, there are two different style backends: servo and gecko. Moving element between documents with different style backends can easily lead to security hazard, and thus we put a MOZ_CRASH there.

With bug 1400540, content page is not expected to create any document with gecko backend, because we only make a document use gecko backend when it uses system principal or it is a XUL document. But content script of browser or extensions may still create such document, so technically this can happen, and it does happen (see bug 1402589).

We would like to have more information about when this happens (e.g. what browser feature, extension can trigger this), so that we have idea how to proceed (e.g. fix the feature to use another backend, whitelist corresponding pages, or contact extension author to do something).

This patch adds some diagnosis information with crash report for that. The additional information can contain sensitive data, so it's probably worth a data collection review. Specifically, it contains the original URL of the document with Gecko backend (so either system principal or XUL), and the URL is sanitized to scheme only if the scheme is http/https/ftp/file/data. It also contains the content type of the document with Gecko backend.
Attachment #8912062 - Flags: feedback?(francois)
I'm marking FF57 as affected in case we decide to take an uplift.
status-firefox57: --- → affected
Priority: -- → P3
Comment on attachment 8912061 [details]
Bug 1403024 part 1 - Add nsContentUtils::SchemeIs helper function.

https://reviewboard.mozilla.org/r/183442/#review188594
Attachment #8912061 - Flags: review?(bobbyholley) → review+
Attachment #8912062 - Flags: review?(bobbyholley) → review?(cam)
Comment on attachment 8912062 [details]
Bug 1403024 part 2 - Add more diagnosis information before crashing for adopting element across style backend.

https://reviewboard.mozilla.org/r/183444/#review188598

::: dom/base/nsDocument.cpp:7875
(Diff revision 2)
> +  nsAutoString contentType;
> +  geckoDoc->GetContentType(contentType);
> +  note.Append(contentType);

I didn't know we could append UTF-16 strings to UTF-8 strings.  I thought we had to use AppendUTF16toUTF8(a, b), or a.Append(NS_ConvertUTF16toUTF8(b))...
Attachment #8912062 - Flags: review?(cam) → review+
Comment on attachment 8912062 [details]
Bug 1403024 part 2 - Add more diagnosis information before crashing for adopting element across style backend.

https://reviewboard.mozilla.org/r/183444/#review188598

> I didn't know we could append UTF-16 strings to UTF-8 strings.  I thought we had to use AppendUTF16toUTF8(a, b), or a.Append(NS_ConvertUTF16toUTF8(b))...

Hmmm... I don't know either, and I just try and it seems it works... But looking at the string code, it doesn't seem to me it should.
Comment on attachment 8912062 [details]
Bug 1403024 part 2 - Add more diagnosis information before crashing for adopting element across style backend.

https://reviewboard.mozilla.org/r/183444/#review188598

> Hmmm... I don't know either, and I just try and it seems it works... But looking at the string code, it doesn't seem to me it should.

So, this would give wrong result. See bug 1403083.

I'll change to use `AppendUTF16ToUTF8` instead.
Attachment #8912062 - Flags: review?(bobbyholley)
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #4)
> Specifically, it contains the original URL of the
> document with Gecko backend (so either system principal or XUL), and the URL
> is sanitized to scheme only if the scheme is http/https/ftp/file/data. It
> also contains the content type of the document with Gecko backend.

Just to check I understand this correctly, if the URL is an internal one (i.e. not a website) then we include the full URL, otherwise, if it's an external URL we only collect the scheme?

Then do we collect the content type in both cases or just for internal URLs?
Flags: needinfo?(xidorn+moz)
(In reply to François Marier [:francois] from comment #11)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #4)
> > Specifically, it contains the original URL of the
> > document with Gecko backend (so either system principal or XUL), and the URL
> > is sanitized to scheme only if the scheme is http/https/ftp/file/data. It
> > also contains the content type of the document with Gecko backend.
> 
> Just to check I understand this correctly, if the URL is an internal one
> (i.e. not a website) then we include the full URL, otherwise, if it's an
> external URL we only collect the scheme?

Yes.

> Then do we collect the content type in both cases or just for internal URLs?

The content type is collected in both cases.
Flags: needinfo?(xidorn+moz)
Comment on attachment 8912062 [details]
Bug 1403024 part 2 - Add more diagnosis information before crashing for adopting element across style backend.

For data collection review.
Attachment #8912062 - Flags: feedback?(francois)
Attachment #8912062 - Flags: review?(francois)
Comment on attachment 8912062 [details]
Bug 1403024 part 2 - Add more diagnosis information before crashing for adopting element across style backend.

https://reviewboard.mozilla.org/r/183444/#review189438

::: dom/base/nsDocument.cpp:7883
(Diff revision 3)
> +  if (nsIURI* geckoURI = geckoDoc->GetOriginalURI()) {
> +    note.AppendLiteral(", url: ");
> +    if (nsContentUtils::SchemeIs(geckoURI, "http") ||
> +        nsContentUtils::SchemeIs(geckoURI, "https") ||
> +        nsContentUtils::SchemeIs(geckoURI, "file") ||
> +        nsContentUtils::SchemeIs(geckoURI, "ftp")) {

Should we worry about `view-source:`, `jar:`, `blob:`, `websocket`?

::: dom/base/nsDocument.cpp:7890
(Diff revision 3)
> +      geckoURI->GetScheme(scheme);
> +      note.Append(scheme);
> +      note.AppendLiteral("://*");
> +    } else if (nsContentUtils::SchemeIs(geckoURI, "data")) {
> +      note.AppendLiteral("data:*");
> +    } else {

In order to ensure that we're not collecting any browsing data or user data, is there a way you can whitelist the schemes that are valid for internal URLs?

So something like:

    } else if (scheme in ['res', 'about', ...]) {
        // send full URL of internal page with a known scheme
    } else {
        // send only the scheme for internal pages with unexpected schemes
    }
Attachment #8912062 - Flags: review?(francois)
Attachment #8912062 - Flags: feedback?(francois)
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #0)
> This bug is for adding diagnosis information for bug 1402589.
> 
> See bug 1402589 comment 2 and discussion in
> http://logs.glob.uno/?c=mozilla%23servo#c757815

The discussion is http://logs.glob.uno/?c=mozilla%23servo&s=25+Sep+2017&e=25+Sep+2017#c757815
(too bad that the link isn't permanent)
Comment on attachment 8912062 [details]
Bug 1403024 part 2 - Add more diagnosis information before crashing for adopting element across style backend.

https://reviewboard.mozilla.org/r/183444/#review189482

With the whitelist of known internal schemes, this should all be Category 2 data.

datareview+
Attachment #8912062 - Flags: review?(francois) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cf08f9815613
part 1 - Add nsContentUtils::SchemeIs helper function. r=bholley
https://hg.mozilla.org/integration/autoland/rev/b8b62d3ba379
part 2 - Add more diagnosis information before crashing for adopting element across style backend. r=francois,heycam
Comment on attachment 8912062 [details]
Bug 1403024 part 2 - Add more diagnosis information before crashing for adopting element across style backend.

Approval Request Comment
[Feature/Bug causing the regression]: this is for collecting information for diagnose bug 1402589
[User impact if declined]: may not have enough info for fixing bug 1402589
[Is this code covered by automated tests?]: not relevant
[Has the fix been verified in Nightly?]: just landed
[Needs manual test from QE? If yes, steps to reproduce]: n/a
[List of other uplifts needed for the feature/fix]: both patches in this bug
[Is the change risky?]: no
[Why is the change risky/not risky?]: it mostly just touches code which is immediately before crashing
[String changes made/needed]: n/a
Attachment #8912062 - Flags: approval-mozilla-beta?
Comment on attachment 8912062 [details]
Bug 1403024 part 2 - Add more diagnosis information before crashing for adopting element across style backend.

More information to fix a crash, taking it
Should be in 57b4, gtb later today
Attachment #8912062 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1404020
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #20)
> [Is this code covered by automated tests?]: not relevant
> [Has the fix been verified in Nightly?]: just landed
> [Needs manual test from QE? If yes, steps to reproduce]: n/a

Setting qe-verify- based on Xidorn's assessment on manual testing needs.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: