Bug 48984 – [Chromium] @media print crash due to paged media support

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Shinichiro Hamaji]

Created attachment 72921 [details]
Test case

Chromium crashes when we are printing the following HTML:

<style>
@media print {
  * {
    display: none;
  }
}
</style>

Chromium bug: http://code.google.com/p/chromium/issues/detail?id=60464

[Yuzo Fujishima]

Created attachment 73225 [details]
Patch

[Shinichiro Hamaji]

Comment on attachment 73225 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73225&action=review

Looks like the expectation file is wrong?

> LayoutTests/printing/page-format-data-display-none-expected.txt:1
> +

Does DRT really output this file?

> WebCore/ChangeLog:6
> +        https://bugs.webkit.org/show_bug.cgi?id=48984

I'd comment why this patch fixes this issue.

[Yuzo Fujishima]

Created attachment 73334 [details]
Added comments

[Yuzo Fujishima]

Thank you for the review.

(In reply to comment #2)
> (From update of attachment 73225 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73225&action=review
> 
> Looks like the expectation file is wrong?
> 
> > LayoutTests/printing/page-format-data-display-none-expected.txt:1
> > +
> 
> Does DRT really output this file?

Yes.

I thought it was weird and spent some time to see if I made any mistakes.
But DRT somehow spit it.

> 
> > WebCore/ChangeLog:6
> > +        https://bugs.webkit.org/show_bug.cgi?id=48984
> 
> I'd comment why this patch fixes this issue.

Added comments there and at styleForPage().

[Shinichiro Hamaji]

> Yes.
> 
> I thought it was weird and spent some time to see if I made any mistakes.
> But DRT somehow spit it.

I see. So, how about adding

document.styleSheets[0].removeRule(0);

after calling pageSizeAndMarginsInPixels to get a sane result?


I'd add a comment which describes how to run the layout test manually.

[Yuzo Fujishima]

Created attachment 73346 [details]
Make expected result cleaner, added manual test instruction

[Yuzo Fujishima]

Thank you for the review, again.

(In reply to comment #5)
> > Yes.
> > 
> > I thought it was weird and spent some time to see if I made any mistakes.
> > But DRT somehow spit it.
> 
> I see. So, how about adding
> 
> document.styleSheets[0].removeRule(0);
> 
> after calling pageSizeAndMarginsInPixels to get a sane result?

Done.

> 
> 
> I'd add a comment which describes how to run the layout test manually.

Done.

[Shinichiro Hamaji]

Comment on attachment 73346 [details]
Make expected result cleaner, added manual test instruction

Looks good. Thanks for updating patch!

[WebKit Commit Bot]

Comment on attachment 73346 [details]
Make expected result cleaner, added manual test instruction

Clearing flags on attachment: 73346

Committed r71621: <http://trac.webkit.org/changeset/71621>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mihai Parparita]

It looks like neither the test_shell nor the Chromium DRT have pageSizeAndMarginsInPixels, I'm going to add a TEXT expectation for this test and open a bug about adding support for it.

[Yuzo Fujishima]

Mihai,

Sorry for the breakage and thank you for filing the bug.