Closed Bug 1320651 Opened 3 years ago Closed 3 years ago

Do not crash in ErrorLoadingBuiltinSheet for every error


(Core :: CSS Parsing and Computation, defect)

Not set



Tracking Status
firefox52 --- fixed
firefox53 --- fixed


(Reporter: wcpan, Assigned: wcpan)




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

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
Only crash for real builtin sheet error. r=bz
Keywords: checkin-needed
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8817091 [details] [diff] [review]

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]

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.