1195978 - getRelativeRuleLine yields wrong result after DOMUtils.parseStyleSheet

Status: RESOLVED FIXED

Description

[Tom Tromey :tromey]

I noticed today that my fix for bug 1013814 causes a regression.
After the patch, if I call DOMUtils.parseStyleSheet on style
sheet coming from a <style> element, then future calls to
getRelativeRuleLine will yield wrong results (in particular they
underflow and are turned into very large numbers due to the use
of unsigned math).

I tracked this down to this line:

 https://dxr.mozilla.org/mozilla-central/source/layout/style/CSSStyleSheet.cpp#2316

which sets the line number to 1 -- but I think it should be the
line number of the <style> element.

I have a patch for this that I'll attach.  It works for my
case but I haven't run the full test suite.

Comment 1

[Tom Tromey :tromey]

Created attachment 8649469 [details] [diff] [review]
set line number when re-parsing sheet

Here's the patch.

Comment 2

[Tom Tromey :tromey]

Comment on attachment 8649469 [details] [diff] [review]
set line number when re-parsing sheet

I am wondering if this seems like a reasonable approach or if there
is some other way I should try.

If it looks ok I will write the test and try a full test run.

Comment 3

[Cameron McCormack (:heycam)]

Comment on attachment 8649469 [details] [diff] [review]
set line number when re-parsing sheet

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

Yeah I think this is OK.  We do preserve the line number when the sheet's text is modified through the DOM.

Comment 4

[Tom Tromey :tromey]

Created attachment 8655680 [details] [diff] [review]
set line number when re-parsing sheet

Patch with test.

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 8655680 [details] [diff] [review]
set line number when re-parsing sheet

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

::: layout/inspector/tests/test_parseStyleSheet.html
@@ +8,5 @@
> +    <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +    <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +    <style>
> +      body {
> +      color: red;

Indentation looks off here.

Comment 6

[Tom Tromey :tromey]

Created attachment 8656025 [details] [diff] [review]
set line number when re-parsing sheet

Updated per review.

Comment 7

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=784259ef3991

Comment 8

[Tom Tromey :tromey]

Removing checkin-needed for now because the patch had a conflict during a rebase.

Comment 9

[Tom Tromey :tromey]

Created attachment 8656556 [details] [diff] [review]
set line number when re-parsing sheet

Rebased.

Comment 10

[Carsten Book [:Tomcat]]

Hi Tom, this patch failed to apply:

adding 1195978 to series file
renamed 1195978 -> Bug-1195978---set-line-number-when-re-parsing-shee.patch
applying Bug-1195978---set-line-number-when-re-parsing-shee.patch
patching file layout/style/CSSStyleSheet.cpp
Hunk #2 FAILED at 2309
1 out of 2 hunks FAILED -- saving rejects to file layout/style/CSSStyleSheet.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh Bug-1195978---set-line-number-when-re-parsing-shee.patch

could you take a look, thanks!

Comment 11

[Tom Tromey :tromey]

(In reply to Carsten Book [:Tomcat] from comment #10)
> Hi Tom, this patch failed to apply:
> 
> adding 1195978 to series file
> renamed 1195978 -> Bug-1195978---set-line-number-when-re-parsing-shee.patch
> applying Bug-1195978---set-line-number-when-re-parsing-shee.patch
> patching file layout/style/CSSStyleSheet.cpp
> Hunk #2 FAILED at 2309
> 1 out of 2 hunks FAILED -- saving rejects to file
> layout/style/CSSStyleSheet.cpp.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working directory
> errors during apply, please fix and refresh
> Bug-1195978---set-line-number-when-re-parsing-shee.patch
> 
> could you take a look, thanks!

Hah, the patch that caused me to do a rebase (comment #9) was since reverted.
I'll attach the replacement patch shortly.

Comment 12

[Tom Tromey :tromey]

Created attachment 8657128 [details] [diff] [review]
set line number when re-parsing sheet

Rebased for the revert.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e33c66fc2ca

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8e33c66fc2ca