Closed Bug 1320651 Opened 3 years ago Closed 3 years ago

Do not crash in ErrorLoadingBuiltinSheet for every error

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: wcpan, Assigned: wcpan)

References

Details

Attachments

(2 files, 2 obsolete files)

Maybe ErrorLoadingBuiltinSheet should not crash for non-critical error.

For example, in bug 1046166, content process may have no permission to read userContent.css, but instead of crashing the process, sending the error message to console service should be enough.

Maybe we can only crash for eAgentSheetFeatures. As for other parsing mode, just send the error message to nsIConsoleService.
That sounds fine.  Failing to load userContent.css and userChrome.css is not critical, though the UA sheets failing to load is.  I probably prefer a separate argument that explicitly indicates whether this is a critical style sheet load, rather than using eAgentSheetFeatures, though.
Comment on attachment 8817079 [details] [diff] [review]
0001-Bug-1320651-Only-crash-for-real-builtin-sheet-error..patch

r=me, though using an enum (e.g. a FailureAction enum with "Crash" and "LogToConsole" as values) instead of a boolean might be nice.
Flags: needinfo?(bzbarsky)
Attachment #8817079 - Flags: review+
Change the diff context to 8 lines.
Attachment #8817079 - Attachment is obsolete: true
Assignee: nobody → wpan
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e325da15c78
Only crash for real builtin sheet error. r=bz
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1e325da15c78
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8817091 [details] [diff] [review]
0001-Bug-1320651-Only-crash-for-real-builtin-sheet-error.-v3.patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1046166
[User impact if declined]: Their tabs might crash immediately because sandbox refuses to load the sheet.
[Is this code covered by automated tests?]: Profiles without userChrome.css/userContent.css has been covered. Manually tested with userChrome.css/userContent.css.
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: bug 1046166
[Is the change risky?]: no
[Why is the change risky/not risky?]: It prevents crashes from some cases, which will improve user experience.
[String changes made/needed]:
Attachment #8817091 - Flags: approval-mozilla-aurora?
Comment on attachment 8817091 [details] [diff] [review]
0001-Bug-1320651-Only-crash-for-real-builtin-sheet-error.-v3.patch

don't crash on failure to load user stylesheet, for aurora52
Attachment #8817091 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
needs rebasing for aurora
Flags: needinfo?(wpan)
Flags: needinfo?(cbook)
You need to log in before you can comment on or make changes to this bug.