Closed Bug 1406474 Opened 7 years ago Closed 7 years ago

stylo: gfxUserFontCache::Entry's copy constructor doesn't transfer its mAllowedFontSets member

Categories

(Core :: CSS Parsing and Computation, defect, P2)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- wontfix
firefox57 + fixed
firefox58 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(1 file)

provide gfxUserFontSet::Entry with a move constructor
1.44 KB, patch
heycam
: review+
ritu
: approval-mozilla-beta+
Details | Diff | Splinter Review
[Tracking Requested - why for this release]: Original bug was a stylo-related issue, we probably want to fix this for the release that will stylo-by-default. gfxUserFontCache::Entry::mAllowedFontSets is not touched by gfxUserFontCache(gfxUserFontCache&&), which means that if gfxUserFontCache is stored in a hashtable, and the hashtable gets resized, we're going to just drop any data in mAllowedFontSets on the floor. I don't think this is intended, and it can result in surprising behavior.
(In reply to Nathan Froyd [:froydnj] from comment #0) > [Tracking Requested - why for this release]: Original bug was a > stylo-related issue, we probably want to fix this for the release that will > stylo-by-default. > > gfxUserFontCache::Entry::mAllowedFontSets is not touched by > gfxUserFontCache(gfxUserFontCache&&), which means that if gfxUserFontCache > is stored in a hashtable, and the hashtable gets resized, we're going to > just drop any data in mAllowedFontSets on the floor. I don't think this is > intended, and it can result in surprising behavior. Sigh, clearly we should be talking about gfxUserFontCache::Entry in the above.
Assignee: nobody → nfroyd
This change makes moving Entry around more efficient, and also copies the mAllowedFontSets member as a ride-along bonus fix.
Attachment #8916071 - Flags: review?(cam)
Comment on attachment 8916071 [details] [diff] [review] provide gfxUserFontSet::Entry with a move constructor Review of attachment 8916071 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Actually, can we make it ALLOW_MEMMOVE = true too?
Attachment #8916071 - Flags: review?(cam) → review+
Pushed by nfroyd@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1779da025280 provide gfxUserFontSet::Entry with a move constructor; r=heycam
Backed out for crashing in reftest and web-platform-tests, e.g. layout/reftests/font-face/download-2-big.html: https://hg.mozilla.org/integration/mozilla-inbound/rev/72e0eea22e345998b3b2ffb27ca17aa10185bca8 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7e79bcb1f22561aae1c60143afe66ecdd6df4e92&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=exception&filter-resultStatus=retry Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=135712635&repo=mozilla-inbound [task 2017-10-09T15:10:32.283Z] 15:10:32 INFO - REFTEST TEST-START | file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/font-face/download-2-big.html == http://localhost:36904/1507561701282/215/font-face/download-2-big-otf.html [task 2017-10-09T15:10:32.285Z] 15:10:32 INFO - REFTEST TEST-LOAD | http://localhost:36904/1507561701282/215/font-face/download-2-big.html | 790 / 1948 (40%) [task 2017-10-09T15:10:32.491Z] 15:10:32 INFO - REFTEST TEST-LOAD | http://localhost:36904/1507561701282/215/font-face/download-2-big-otf.html | 790 / 1948 (40%) [task 2017-10-09T15:10:32.762Z] 15:10:32 INFO - ###!!! [Parent][MessageChannel] Error: (msgtype=0x150083,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv [task 2017-10-09T15:10:32.779Z] 15:10:32 INFO - [Parent 1004, Gecko_IOThread] WARNING: waitpid failed pid:1106 errno:10: file /builds/worker/workspace/build/src/ipc/chromium/src/base/process_util_posix.cc, line 276 [task 2017-10-09T15:16:02.795Z] 15:16:02 ERROR - REFTEST ERROR | file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/font-face/download-2-big.html | application timed out after 330 seconds with no output [task 2017-10-09T15:16:02.797Z] 15:16:02 ERROR - REFTEST ERROR | Force-terminating active process(es). [task 2017-10-09T15:16:02.799Z] 15:16:02 INFO - REFTEST TEST-INFO | started process screentopng [task 2017-10-09T15:16:03.152Z] 15:16:03 INFO - REFTEST TEST-INFO | screentopng: exit 0 [task 2017-10-09T15:16:03.426Z] 15:16:03 ERROR - TEST-UNEXPECTED-FAIL | file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/font-face/download-2-big.html | application terminated with exit code 6 [task 2017-10-09T15:16:03.427Z] 15:16:03 INFO - REFTEST INFO | Downloading symbols from: https://queue.taskcluster.net/v1/task/OO5-cxmqSeitQcqdDnU7rg/artifacts/public/build/target.crashreporter-symbols.zip [task 2017-10-09T15:16:08.715Z] 15:16:08 INFO - REFTEST INFO | Copy/paste: /usr/local/bin/linux64-minidump_stackwalk /tmp/tmpT_6kfR.mozrunner/minidumps/1d76d66f-7035-8063-8dec-5c2c5f871578.dmp /tmp/tmpcWdg4x [task 2017-10-09T15:16:17.392Z] 15:16:17 INFO - REFTEST INFO | Saved minidump as /builds/worker/workspace/build/blobber_upload_dir/1d76d66f-7035-8063-8dec-5c2c5f871578.dmp [task 2017-10-09T15:16:17.394Z] 15:16:17 INFO - REFTEST INFO | Saved app info as /builds/worker/workspace/build/blobber_upload_dir/1d76d66f-7035-8063-8dec-5c2c5f871578.extra [task 2017-10-09T15:16:17.447Z] 15:16:17 INFO - REFTEST PROCESS-CRASH | file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/font-face/download-2-big.html | application crashed [@ libc-2.23.so + 0xfb70d] [task 2017-10-09T15:16:17.448Z] 15:16:17 INFO - Crash dump filename: /tmp/tmpT_6kfR.mozrunner/minidumps/1d76d66f-7035-8063-8dec-5c2c5f871578.dmp [task 2017-10-09T15:16:17.448Z] 15:16:17 INFO - Operating system: Linux [task 2017-10-09T15:16:17.449Z] 15:16:17 INFO - 0.0.0 Linux 3.13.0-112-generic #159-Ubuntu SMP Fri Mar 3 15:26:07 UTC 2017 x86_64 [task 2017-10-09T15:16:17.450Z] 15:16:17 INFO - CPU: amd64 [task 2017-10-09T15:16:17.451Z] 15:16:17 INFO - family 6 model 62 stepping 4 [task 2017-10-09T15:16:17.451Z] 15:16:17 INFO - 2 CPUs [task 2017-10-09T15:16:17.452Z] 15:16:17 INFO - [task 2017-10-09T15:16:17.453Z] 15:16:17 INFO - GPU: UNKNOWN [task 2017-10-09T15:16:17.454Z] 15:16:17 INFO - [task 2017-10-09T15:16:17.454Z] 15:16:17 INFO - Crash reason: SIGABRT [task 2017-10-09T15:16:17.455Z] 15:16:17 INFO - Crash address: 0x3e8000003e2 [task 2017-10-09T15:16:17.456Z] 15:16:17 INFO - Process uptime: not available [task 2017-10-09T15:16:17.457Z] 15:16:17 INFO - [task 2017-10-09T15:16:17.457Z] 15:16:17 INFO - Thread 0 (crashed) [task 2017-10-09T15:16:17.458Z] 15:16:17 INFO - 0 libc-2.23.so + 0xfb70d [task 2017-10-09T15:16:17.459Z] 15:16:17 INFO - rax = 0xfffffffffffffffc rdx = 0x00000000ffffffff [task 2017-10-09T15:16:17.459Z] 15:16:17 INFO - rcx = 0xffffffffffffffff rbx = 0x00007f06e4410100 [task 2017-10-09T15:16:17.460Z] 15:16:17 INFO - rsi = 0x0000000000000005 rdi = 0x00007f06e4410100 [task 2017-10-09T15:16:17.461Z] 15:16:17 INFO - rbp = 0x00007ffd2dadae10 rsp = 0x00007ffd2dadade0 [task 2017-10-09T15:16:17.462Z] 15:16:17 INFO - r8 = 0x0000000000000005 r9 = 0x0000000000000001 [task 2017-10-09T15:16:17.462Z] 15:16:17 INFO - r10 = 0x00007f06e7fff260 r11 = 0x0000000000000293 [task 2017-10-09T15:16:17.463Z] 15:16:17 INFO - r12 = 0x0000000000000005 r13 = 0x00000000ffffffff [task 2017-10-09T15:16:17.464Z] 15:16:17 INFO - r14 = 0x00007f070a6aaaf0 r15 = 0x0000000000000005 [task 2017-10-09T15:16:17.465Z] 15:16:17 INFO - rip = 0x00007f0718ee670d [task 2017-10-09T15:16:17.465Z] 15:16:17 INFO - Found by: given as instruction pointer in context [task 2017-10-09T15:16:17.466Z] 15:16:17 INFO - 1 libxul.so!PollWrapper [nsAppShell.cpp:7e79bcb1f225 : 52 + 0xf] [task 2017-10-09T15:16:17.467Z] 15:16:17 INFO - rbp = 0x00007ffd2dadae10 rsp = 0x00007ffd2dadadf0 [task 2017-10-09T15:16:17.467Z] 15:16:17 INFO - rip = 0x00007f070a6aab1f [task 2017-10-09T15:16:17.468Z] 15:16:17 INFO - Found by: stack scanning [task 2017-10-09T15:16:17.469Z] 15:16:17 INFO - 2 libglib-2.0.so.0.4800.2 + 0x4a38c [task 2017-10-09T15:16:17.470Z] 15:16:17 INFO - rbx = 0x00007f0718bd1480 rbp = 0x0000000000000005 [task 2017-10-09T15:16:17.470Z] 15:16:17 INFO - rsp = 0x00007ffd2dadae20 r12 = 0x00007f06e4410100 [task 2017-10-09T15:16:17.471Z] 15:16:17 INFO - r13 = 0x00000000ffffffff rip = 0x00007f0713cba38c [task 2017-10-09T15:16:17.472Z] 15:16:17 INFO - Found by: call frame info [task 2017-10-09T15:16:17.472Z] 15:16:17 INFO - 3 libglib-2.0.so.0.4800.2 + 0x4a49c [task 2017-10-09T15:16:17.473Z] 15:16:17 INFO - rsp = 0x00007ffd2dadae80 rip = 0x00007f0713cba49c [task 2017-10-09T15:16:17.474Z] 15:16:17 INFO - Found by: stack scanning [task 2017-10-09T15:16:17.474Z] 15:16:17 INFO - 4 libxul.so!nsAppShell::ProcessNextNativeEvent [nsAppShell.cpp:7e79bcb1f225 : 282 + 0x5] [task 2017-10-09T15:16:17.475Z] 15:16:17 INFO - rsp = 0x00007ffd2dadaea0 rip = 0x00007f070a6aab6f [task 2017-10-09T15:16:17.475Z] 15:16:17 INFO - Found by: stack scanning [task 2017-10-09T15:16:17.476Z] 15:16:17 INFO - 5 libxul.so!nsBaseAppShell::DoProcessNextNativeEvent [nsBaseAppShell.cpp:7e79bcb1f225 : 140 + 0x10] [task 2017-10-09T15:16:17.477Z] 15:16:17 INFO - rsp = 0x00007ffd2dadaeb0 rip = 0x00007f070a677aec [task 2017-10-09T15:16:17.477Z] 15:16:17 INFO - Found by: stack scanning [task 2017-10-09T15:16:17.478Z] 15:16:17 INFO - 6 libxul.so!nsBaseAppShell::OnProcessNextEvent [nsBaseAppShell.cpp:7e79bcb1f225 : 291 + 0x8] [task 2017-10-09T15:16:17.479Z] 15:16:17 INFO - rsp = 0x00007ffd2dadaee0 rip = 0x00007f070a677ca0 [task 2017-10-09T15:16:17.480Z] 15:16:17 INFO - Found by: stack scanning [task 2017-10-09T15:16:17.480Z] 15:16:17 INFO - 7 libxul.so!nsThread::ProcessNextEvent [nsThread.cpp:7e79bcb1f225 : 950 + 0xf] [task 2017-10-09T15:16:17.481Z] 15:16:17 INFO - rsp = 0x00007ffd2dadaf30 rip = 0x00007f070875d501 [task 2017-10-09T15:16:17.482Z] 15:16:17 INFO - Found by: stack scanning [task 2017-10-09T15:16:17.482Z] 15:16:17 INFO - 8 firefox!mozilla::PrintfTarget::fill_n [Printf.cpp:7e79bcb1f225 : 134 + 0x6] [task 2017-10-09T15:16:17.483Z] 15:16:17 INFO - rsp = 0x00007ffd2dadaf80 rip = 0x0000000000416d01 [task 2017-10-09T15:16:17.484Z] 15:16:17 INFO - Found by: stack scanning [task 2017-10-09T15:16:17.484Z] 15:16:17 INFO - 9 libxul.so!_fini + 0x20fb48 [task 2017-10-09T15:16:17.485Z] 15:16:17 INFO - rsp = 0x00007ffd2dadafc0 rip = 0x00007f070c8ce000 [task 2017-10-09T15:16:17.486Z] 15:16:17 INFO - Found by: stack scanning [task 2017-10-09T15:16:17.486Z] 15:16:17 INFO - 10 libxul.so!_fini + 0x201b48 [task 2017-10-09T15:16:17.487Z] 15:16:17 INFO - rsp = 0x00007ffd2dadafd0 rip = 0x00007f070c8c0000 [task 2017-10-09T15:16:17.488Z] 15:16:17 INFO - Found by: stack scanning [task 2017-10-09T15:16:17.488Z] 15:16:17 INFO - 11 firefox!mozilla::PrintfTarget::cvt_l [Printf.cpp:7e79bcb1f225 : 213 + 0x16] [task 2017-10-09T15:16:17.489Z] 15:16:17 INFO - rsp = 0x00007ffd2dadafe0 rip = 0x0000000000416f69 [task 2017-10-09T15:16:17.490Z] 15:16:17 INFO - Found by: stack scanning [task 2017-10-09T15:16:17.490Z] 15:16:17 INFO - 12 firefox!_fini + 0x1b1c [task 2017-10-09T15:16:17.491Z] 15:16:17 INFO - rsp = 0x00007ffd2dadaff0 rip = 0x000000000042f7b4 [task 2017-10-09T15:16:17.492Z] 15:16:17 INFO - Found by: stack scanning [task 2017-10-09T15:16:17.493Z] 15:16:17 INFO - 13 libxul.so!nsTSubstring<char>::ReplaceASCII [nsTSubstring.cpp:7e79bcb1f225 : 656 + 0xb] [task 2017-10-09T15:16:17.493Z] 15:16:17 INFO - rsp = 0x00007ffd2dadb000 rip = 0x00007f07086d5000 [task 2017-10-09T15:16:17.494Z] 15:16:17 INFO - Found by: stack scanning [task 2017-10-09T15:16:17.495Z] 15:16:17 INFO - 14 firefox!_fini + 0x1a69 [task 2017-10-09T15:16:17.495Z] 15:16:17 INFO - rsp = 0x00007ffd2dadb010 rip = 0x000000000042f701 [task 2017-10-09T15:16:17.496Z] 15:16:17 INFO - Found by: stack scanning [task 2017-10-09T15:16:17.497Z] 15:16:17 INFO - 15 firefox!arena_t::SplitRun [rb.h:7e79bcb1f225 : 184 + 0x5] [task 2017-10-09T15:16:17.497Z] 15:16:17 INFO - rsp = 0x00007ffd2dadb070 rip = 0x000000000040eabb [task 2017-10-09T15:16:17.498Z] 15:16:17 INFO - Found by: stack scanning
Flags: needinfo?(nfroyd)
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #5) > Backed out for crashing in reftest and web-platform-tests, e.g. > layout/reftests/font-face/download-2-big.html: > > https://hg.mozilla.org/integration/mozilla-inbound/rev/ > 72e0eea22e345998b3b2ffb27ca17aa10185bca8 > > Push with failures: > https://treeherder.mozilla.org/#/jobs?repo=mozilla- > inbound&revision=7e79bcb1f22561aae1c60143afe66ecdd6df4e92&filter- > resultStatus=testfailed&filter-resultStatus=busted&filter- > resultStatus=usercancel&filter-resultStatus=runnable&filter- > resultStatus=exception&filter-resultStatus=retry > Failure log: > https://treeherder.mozilla.org/logviewer.html#?job_id=135712635&repo=mozilla- > inbound I don't really understand the failure here. The logs say that we are crashing at: https://hg.mozilla.org/integration/mozilla-inbound/file/1779da025280d3521be05f5c4275095f6224d2f6/xpcom/ds/PLDHashTable.cpp#l519 From which I can only conclude that mOps->hashKey is nullptr. But this is pretty weird, because we always initialize that field in nsTHashtable: https://hg.mozilla.org/integration/mozilla-inbound/file/1779da025280d3521be05f5c4275095f6224d2f6/xpcom/ds/nsTHashtable.h#l400 So how is that possibly ending up as nullptr? The stack says we're doing this all on the main thread, so there's no weirdness coming from multiple threads in Stylo...unless we're touching the font cache from Stylo on the *previous* parallel traversal and we've somehow left nullptrs strewn about, which the pretraversal above picks up? heycam, do you have a clue what might be going on here? Do we touch this data from multiple threads anywhere?
Flags: needinfo?(nfroyd) → needinfo?(cam)
Never mind, this is PEBKAC. I see what the problem is.
Flags: needinfo?(cam)
Pushed by nfroyd@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2b181f6ba9d4 provide gfxUserFontSet::Entry with a move constructor; r=heycam
(In reply to Pulsebot from comment #8) > Pushed by nfroyd@mozilla.com: > https://hg.mozilla.org/integration/mozilla-inbound/rev/2b181f6ba9d4 > provide gfxUserFontSet::Entry with a move constructor; r=heycam It works better to initialize members in move constructors from the object you're moving from, rather than the object you're initializing, as the previous version was doing...
(In reply to Nathan Froyd [:froydnj] from comment #0) > [Tracking Requested - why for this release]: Original bug was a > stylo-related issue, we probably want to fix this for the release that will > stylo-by-default. > > gfxUserFontCache::Entry::mAllowedFontSets is not touched by > gfxUserFontCache(gfxUserFontCache&&), which means that if gfxUserFontCache > is stored in a hashtable, and the hashtable gets resized, we're going to > just drop any data in mAllowedFontSets on the floor. I don't think this is > intended, and it can result in surprising behavior. Hi Nathan - In evaluating this for 57, can you elaborate on the surprising behavior that will be seen here? Will this issue be very visible to end users? Thanks.
Flags: needinfo?(nfroyd)
(In reply to Marcia Knous [:marcia - use ni] from comment #10) > (In reply to Nathan Froyd [:froydnj] from comment #0) > > [Tracking Requested - why for this release]: Original bug was a > > stylo-related issue, we probably want to fix this for the release that will > > stylo-by-default. > > > > gfxUserFontCache::Entry::mAllowedFontSets is not touched by > > gfxUserFontCache(gfxUserFontCache&&), which means that if gfxUserFontCache > > is stored in a hashtable, and the hashtable gets resized, we're going to > > just drop any data in mAllowedFontSets on the floor. I don't think this is > > intended, and it can result in surprising behavior. > > Hi Nathan - In evaluating this for 57, can you elaborate on the surprising > behavior that will be seen here? Will this issue be very visible to end > users? Thanks. Bug 1384741 was about incorrect CSP reports seen when Stylo is being used. I think--not being an expert here on all the interactions--with this bug it's possible that some CSP reports would still get through, depending on the particular page and the fonts that were used. Does that sound right, Cameron?
Flags: needinfo?(nfroyd) → needinfo?(cam)
Yes, that's right. I think we should uplift to avoid server operators seeing spurious CSP reports.
Flags: needinfo?(cam)
https://hg.mozilla.org/mozilla-central/rev/2b181f6ba9d4
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Tracking for 57, please request uplift when you get a chance.
tracking-firefox57: ? → +
Comment on attachment 8916071 [details] [diff] [review] provide gfxUserFontSet::Entry with a move constructor Approval Request Comment [Feature/Bug causing the regression]: bug 1384741 [User impact if declined]: spurious CSP reports [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: no [Needs manual test from QE? If yes, steps to reproduce]: don't think we need to try and trigger this one; the original bug fix did eliminate CSP reports for the reported test case. Trying to write a testcase to trigger the spurious CSP reports this bug would fix would be tricky. [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: This change just makes us do what we thought we were doing all along. [String changes made/needed]: none
Attachment #8916071 - Flags: approval-mozilla-beta?
Comment on attachment 8916071 [details] [diff] [review] provide gfxUserFontSet::Entry with a move constructor Stylo related, seems like a must fix, Beta57+. Selena FYI, if this isn't a must fix, please let me know.
Attachment #8916071 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: