Closed
Bug 1277534
Opened 8 years ago
Closed 8 years ago
Unterminated URLs break the rule-view
Categories
(DevTools :: Inspector: Rules, defect, P1)
Tracking
(firefox49 verified, firefox50 verified)
People
(Reporter: pbro, Assigned: tromey)
References
Details
(Keywords: regression, Whiteboard: [devtools-html])
Attachments
(1 file)
|
(deleted),
text/x-review-board-request
|
pbro
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details |
STR:
- open the following URL in a new tab: data:text/html,<style>body {background: red url(unterminated}</style>
- notice that even if the url is invalid and the css property is unterminated, the color red applies to the background, so the browser does apply the property
- now open the inspector (and the css rule view sidebar)
- make sure the <body> is selected in the inspector
==> The rule-view is empty
==> It should show the rule as:
body {
background: #f00 url(unterminated});
}
Instead, the following error is thrown:
TypeError: unterminated} is not a valid URL.
Stack trace:
OutputParser.prototype._appendURL@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/output-parser.js:532:16
This works fine in FF48, but fails in FF49. And this is a regression from bug 1265813 (Replace nsIIOService with URL in devtools).
|
Reporter | |
Comment 1•8 years ago
|
||
I think we should just silently fail for invalid URLs.
I'm unsure what made it work before, but a try/catch around `new URL` in `_appendURL` in `output-parser.js` would work fine.
What do you think Tom?
Flags: needinfo?(ttromey)
|
||
Updated•8 years ago
|
Blocks: devtools-html-3
Whiteboard: [devtools-html] [triage]
|
|
||
Updated•8 years ago
|
Flags: qe-verify+
Priority: -- → P2
QA Contact: alexandra.lucinet
Whiteboard: [devtools-html] [triage] → [devtools-html]
|
|
Reporter | |
Updated•8 years ago
|
Blocks: top-inspector-bugs
|
Assignee | |
Comment 2•8 years ago
|
||
Sorry about this.
The plan sounds reasonable, I'll take a look.
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Flags: needinfo?(ttromey)
|
|
||
Updated•8 years ago
|
Iteration: --- → 49.3 - Jun 6
Priority: P2 → P1
|
|
Assignee | |
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/57936/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57936/
Attachment #8760290 -
Flags: review?(pbrosset)
|
|
Reporter | |
Comment 4•8 years ago
|
||
Comment on attachment 8760290 [details]
Bug 1277534 - avoid output-parser failure with invalid URL;
https://reviewboard.mozilla.org/r/57936/#review54764
Thanks Tom!
Attachment #8760290 -
Flags: review?(pbrosset) → review+
|
|
Assignee | |
Comment 5•8 years ago
|
||
|
|
Assignee | |
Comment 6•8 years ago
|
||
I was concerned about the "cl" failure but I see it in an earlier try run on the branch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5310316fc380&selectedJob=22042711
... so I think it wasn't introduce by this patch.
Keywords: checkin-needed
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/529f3c1c7c1f
avoid output-parser failure with invalid URL; r=pbro
Keywords: checkin-needed
|
||
Comment 8•8 years ago
|
||
| bugherder | ||
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
|
|
||
Comment 9•8 years ago
|
||
| bugherder | ||
|
|
Reporter | |
Updated•8 years ago
|
No longer blocks: top-inspector-bugs
Since this is a regression from 49, can you request uplift to aurora?
I'm not sure of the risk here, but it would be nice to ship 49 without this issue.
Flags: needinfo?(ttromey)
|
|
Assignee | |
Comment 11•8 years ago
|
||
Comment on attachment 8760290 [details]
Bug 1277534 - avoid output-parser failure with invalid URL;
Approval Request Comment
[Feature/regressing bug #]: Bug 1265813 - replace uses of nsIIOService
[User impact if declined]:
Certain invalid URLs in a style will prevent the inspector from working --
the inspector panel will wind up being blank.
[Describe test coverage new/current, TreeHerder]:
There's a new regression test for the patch.
[Risks and why]:
The patch is low risk. It adds a try/catch to avoid an unexpected exception
crashing the inspector. There is also a new test case to test the specific
failure mode. Furthermore this patch is unlikely to add any new failures;
and if one is added, the code in question will simply go to a pre-existing
fallback.
[String/UUID change made/needed]: N/A
Flags: needinfo?(ttromey)
Attachment #8760290 -
Flags: approval-mozilla-aurora?
Comment on attachment 8760290 [details]
Bug 1277534 - avoid output-parser failure with invalid URL;
Fix for regressions, has new tests, is totally awesome, so let's put it in aurora.
Attachment #8760290 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
|
|
||
Comment 13•8 years ago
|
||
|
|
||
Updated•8 years ago
|
QA Contact: adalucin → petruta.rasa
|
||
Comment 14•8 years ago
|
||
Verified that using Firefox 49 beta 1 and latest Aurora 50.0a2 2016-08-08 under Win 10 64-bit, Ubuntu 14.04 and Mac OS X 10.11, the color applies to background, there is no error in Browser Console and <body> shows the following rule:
body {
background: red url(unterminated});
}
Marking as verified.
|
|
||
Updated•8 years ago
|
QA Whiteboard: [qe-dthtml]
Flags: qe-verify+
|
||
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•