Closed Bug 1293252 Opened 3 years ago Closed 3 years ago

Unit-less CSS length value is wrongly applied if the page was open in background tab. (On getpen.ru some elements are not loaded properly when a link is opened as a background tab)

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla51
Tracking Status
firefox48 --- wontfix
firefox49 + wontfix
firefox-esr45 --- wontfix
firefox50 + fixed
firefox51 + verified

People

(Reporter: ajfhajf, Assigned: hsivonen)

References

()

Details

(Keywords: regression, reproducible, testcase, Whiteboard: [necko-active])

Attachments

(6 files)

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20160808030441

Steps to reproduce:

Open http://getpen.ru/ in the background tab
If you see a blank page at the center of the screen, reload the page in order to make Nightly render it correctly. That's the bug.

If you don't see a blank page, open ~5 tabs in the background by clicking on the images of pens, wait until they load

Result:
Contents of those 5 tabs is rendered incorrectly with blank page at the center of the screen instead of the image, comments and description. After reload the pages are displayed correctly. Bug is not reproduced in the latest Opera. Also, as far as I remember, bug is reproduced in FF Stable as well. I'll check that later.
Confirmed on my Mac.
Status: UNCONFIRMED → NEW
Ever confirmed: true
See Also: → 1282215
http://getpen.ru/css/screen.css line77 has an invalid entry:
.last {margin-right:0;padding-right:0;margin-top:-11110} (unit is missing for margin-top)

I think it is affecting the page only at first load as a background tab.
Attached image bug1293252_rules.PNG
Given by the clue in bug 1282215 comment 9, I think I have been seeing this issue for many years at Cracked.com.

STR:
1. Open a multi-page cracked.com article in a new background tab, e.g. http://www.cracked.com/article_20276_5-famous-kids-characters-you-didnt-know-were-propaganda.html.
2. Scroll past the main article.
3. Observe the heading titled "Recommended For Your Pleasure".

Actual result:
The CSS letter-spacing:-30 is applied as letter-spacing:-30px even if it is crossed out in the Rule panel.
Attached image bug1293252_computed.PNG
This rule is only applied when opening the tab as a background tab.
Oh nice find Fanolian. Do you think you can come up with a minimal test case and attach it here?
Flags: needinfo?(Fanolian+bugzilla)
(In reply to Mike Taylor [:miketaylr] from comment #5)
> Oh nice find Fanolian. Do you think you can come up with a minimal test case
> and attach it here?

Sorry I don't know writing codes. Perhaps I can find some help in the forum.
Flags: needinfo?(Fanolian+bugzilla)
Keywords: reproducible
Product: Firefox → Core
[Tracking Requested - why for this release]: Serious flaws related to product reliability
Reproduced on
https://hg.mozilla.org/mozilla-central/rev/e78975b53563d80c99ebfbdf8a9fbf6b829a8a48
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0 ID:20160808030441


Str:
1. Extract zip to a local folder
2. Open "Bug 1293252-main.html"
3. Middle click "link" to open in background tab.
4. Swith to the tab, and observe margin-left

Actual Results:
margin-left is wrongly applied.

Expected Results:
Unitless value of margin-left should be ignored.
Attachment #8779240 - Attachment description: testcase → testcase (slow pc may be required to reproduce)
I tried to find regression window on WindowsXP SP1 32bit 4@2.0GHzCPU RAM3GB on VMWare Player.

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e7e9304438df&tochange=16ff50091d32

Suspect: Bug 1024898
Blocks: 1024898
Flags: needinfo?(sworkman)
Flags: needinfo?(jduell.mcbugs)
Via local build,
Last Good: 97fa54d04c48
First Bad: 16ff50091d32

Triggered by : 16ff50091d32	Kyle Huey — Bug 1024898: Allow (most) nsBaseChannel subclasses to retarget OnDataAvailable to other threads. r=jduell,sworkman


For now, change the component to the network.
Component: Untriaged → Networking
Keywords: regression
Whiteboard: [necko-active]
Looks like this needs some debugging, and I will be leaving soon. So, I will leave it to the Necko folks to take care of it :)
Clearing my needinfo.
Flags: needinfo?(sworkman)
Has Regression Range: --- → yes
Has STR: --- → yes
Keywords: testcase
Attached file testcase_version2.zip
Bug 1024898 of comment#9 and comment#10 seems only hit the existing bug.

New testcase version2 is more easy to reproduce the problem.

Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7b94359fb653&tochange=e24844e280a0

Suspect: Bug 734015
Blocks: 734015
No longer blocks: 1024898
Component: Networking → DOM: Core & HTML
Flags: needinfo?(hsivonen)
Flags: needinfo?(bugs)
Version: 51 Branch → Trunk
Flags: needinfo?(jduell.mcbugs)
Summary: On getpen.ru some elements are not loaded properly when a link is opened as a background tab → Unit-less CSS length value is wrongly applied if the page was open in background tab. (On getpen.ru some elements are not loaded properly when a link is opened as a background tab)
Hi HsinYi,
Can you help to find someone to investigate this? Not sure if this is related to DOM.
Flags: needinfo?(htsai)
I found that nsCSSParser parse the unit-less numbers as pixel [1] when issue happened, but I have no idea why yet. 

[1] https://dxr.mozilla.org/mozilla-central/rev/fe895421dfbe1f1f8f1fc6a39bb20774423a6d74/layout/style/nsCSSParser.cpp#7842
(In reply to Edgar Chen [:edgar][:echen] from comment #14)
> I found that nsCSSParser parse the unit-less numbers as pixel [1] when issue

It is because css::Loader is in "eCompatibility_NavQuirks" mode during parsing.

> happened, but I have no idea why yet. 
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> fe895421dfbe1f1f8f1fc6a39bb20774423a6d74/layout/style/nsCSSParser.cpp#7842

And the css::Loader's compatibility mode is from HTMLDocument.

When issue happens (at first load as a background tab), the flow likes this,

* 1. nsHTMLDocument was created in eCompatibility_NavQuirks mode by default [1] and started document load.

[1] https://dxr.mozilla.org/mozilla-central/rev/97a52326b06a07930216ebefa5af333271578904/dom/html/nsHTMLDocument.cpp#182

Breakpoint 2, mozilla::css::Loader::SetCompatibilityMode (this=0x7f8475bced40, aCompatMode=eCompatibility_NavQuirks) at /data/mercurial/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/css/Loader.h:207
207       { mCompatMode = aCompatMode; }
(gdb) bt
#0  mozilla::css::Loader::SetCompatibilityMode (this=0x7f8475bced40, aCompatMode=eCompatibility_NavQuirks) at /data/mercurial/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/css/Loader.h:207
#1  0x00007f84a519383c in nsHTMLDocument::StartDocumentLoad (this=0x7f847c1ce000, aCommand=0x7f84a96f69b6 "view", aChannel=0x7f847e5e3700, aLoadGroup=0x7f8478eab670, aContainer=0x7f8478e8b9a8, aDocListener=0x7f847e3b3c08, aReset=true, 
    aSink=0x0) at /data/mercurial/mozilla-central/dom/html/nsHTMLDocument.cpp:578
#2  0x00007f84a6515b67 in nsContentDLF::CreateDocument (this=0x7f848b16cb80, aCommand=0x7f84a96f69b6 "view", aChannel=0x7f847e5e3700, aLoadGroup=0x7f8478eab670, aContainer=0x7f8478e8b9a8, aDocumentCID=..., aDocListener=0x7f847e3b3c08, 
    aContentViewer=0x7ffc2c1758f0) at /data/mercurial/mozilla-central/layout/build/nsContentDLF.cpp:375
#3  0x00007f84a6514f2d in nsContentDLF::CreateInstance (this=0x7f848b16cb80, aCommand=0x7f84a96f69b6 "view", aChannel=0x7f847e5e3700, aLoadGroup=0x7f8478eab670, aContentType=..., aContainer=0x7f8478e8b9a8, aExtraInfo=0x0, 
    aDocListener=0x7f847e3b3c08, aDocViewer=0x7ffc2c1758f0) at /data/mercurial/mozilla-central/layout/build/nsContentDLF.cpp:186
#4  0x00007f84a6842c01 in nsDocShell::NewContentViewerObj (this=0x7f8478e8b800, aContentType=..., aRequest=0x7f847e5e3700, aLoadGroup=0x7f8478eab670, aContentHandler=0x7f847e3b3c08, aViewer=0x7ffc2c1758f0)
    at /data/mercurial/mozilla-central/docshell/base/nsDocShell.cpp:9231
#5  0x00007f84a6841f5c in nsDocShell::CreateContentViewer (this=0x7f8478e8b800, aContentType=..., aRequest=0x7f847e5e3700, aContentHandler=0x7f847e3b3c08) at /data/mercurial/mozilla-central/docshell/base/nsDocShell.cpp:9027
#6  0x00007f84a6819f56 in nsDSURIContentListener::DoContent (this=0x7f8478ba6a40, aContentType=..., aIsContentPreferred=false, aRequest=0x7f847e5e3700, aContentHandler=0x7f847e3b3c08, aAbortProcess=0x7ffc2c175a5e)
    at /data/mercurial/mozilla-central/docshell/base/nsDSURIContentListener.cpp:128
#7  0x00007f84a35c0889 in nsDocumentOpenInfo::TryContentListener (this=0x7f847e3b3be0, aListener=0x7f8478ba6a40, aChannel=0x7f847e5e3700) at /data/mercurial/mozilla-central/uriloader/base/nsURILoader.cpp:724
#8  0x00007f84a35bf145 in nsDocumentOpenInfo::DispatchContent (this=0x7f847e3b3be0, request=0x7f847e5e3700, aCtxt=0x0) at /data/mercurial/mozilla-central/uriloader/base/nsURILoader.cpp:398
#9  0x00007f84a35be809 in nsDocumentOpenInfo::OnStartRequest (this=0x7f847e3b3be0, request=0x7f847e5e3700, aCtxt=0x0) at /data/mercurial/mozilla-central/uriloader/base/nsURILoader.cpp:259
#10 0x00007f84a23aa3f6 in nsBaseChannel::OnStartRequest (this=0x7f847e5e36b0, request=0x7f8475e453c0, ctxt=0x0) at /data/mercurial/mozilla-central/netwerk/base/nsBaseChannel.cpp:809
#11 0x00007f84a23d1b38 in nsInputStreamPump::OnStateStart (this=0x7f8475e453c0) at /data/mercurial/mozilla-central/netwerk/base/nsInputStreamPump.cpp:524
#12 0x00007f84a23d1700 in nsInputStreamPump::OnInputStreamReady (this=0x7f8475e453c0, stream=0x7f848d8a25e0) at /data/mercurial/mozilla-central/netwerk/base/nsInputStreamPump.cpp:426
#13 0x00007f84a22796eb in nsInputStreamReadyEvent::Run (this=0x7f847d43e300) at /data/mercurial/mozilla-central/xpcom/io/nsStreamUtils.cpp:95
#14 0x00007f84a22a1aa7 in nsThread::ProcessNextEvent (this=0x7f84b3b8c300, aMayWait=false, aResult=0x7ffc2c175edf) at /data/mercurial/mozilla-central/xpcom/threads/nsThread.cpp:1058
#15 0x00007f84a230a701 in NS_ProcessNextEvent (aThread=0x7f84b3b8c300, aMayWait=false) at /data/mercurial/mozilla-central/xpcom/glue/nsThreadUtils.cpp:290
#16 0x00007f84a2a5f327 in mozilla::ipc::MessagePump::Run (this=0x7f849ed99800, aDelegate=0x7f84b3b55880) at /data/mercurial/mozilla-central/ipc/glue/MessagePump.cpp:96
#17 0x00007f84a29cf905 in MessageLoop::RunInternal (this=0x7f84b3b55880) at /data/mercurial/mozilla-central/ipc/chromium/src/base/message_loop.cc:232
#18 0x00007f84a29cf89a in MessageLoop::RunHandler (this=0x7f84b3b55880) at /data/mercurial/mozilla-central/ipc/chromium/src/base/message_loop.cc:225
#19 0x00007f84a29cf873 in MessageLoop::Run (this=0x7f84b3b55880) at /data/mercurial/mozilla-central/ipc/chromium/src/base/message_loop.cc:205
#20 0x00007f84a5d34d18 in nsBaseAppShell::Run (this=0x7f8491715c50) at /data/mercurial/mozilla-central/widget/nsBaseAppShell.cpp:156
#21 0x00007f84a6ce6353 in nsAppStartup::Run (this=0x7f84917181f0) at /data/mercurial/mozilla-central/toolkit/components/startup/nsAppStartup.cpp:284
#22 0x00007f84a6da5d0c in XREMain::XRE_mainRun (this=0x7ffc2c1762d0) at /data/mercurial/mozilla-central/toolkit/xre/nsAppRunner.cpp:4302
#23 0x00007f84a6da62a1 in XREMain::XRE_main (this=0x7ffc2c1762d0, argc=4, argv=0x7ffc2c1776c8, aAppData=0x7ffc2c1764f0) at /data/mercurial/mozilla-central/toolkit/xre/nsAppRunner.cpp:4429
#24 0x00007f84a6da6571 in XRE_main (argc=4, argv=0x7ffc2c1776c8, aAppData=0x7ffc2c1764f0, aFlags=0) at /data/mercurial/mozilla-central/toolkit/xre/nsAppRunner.cpp:4520
#25 0x0000000000405a71 in do_main (argc=4, argv=0x7ffc2c1776c8, envp=0x7ffc2c1776f0, xreDirectory=0x7f84b3b6b780) at /data/mercurial/mozilla-central/browser/app/nsBrowserApp.cpp:259
#26 0x0000000000405dd8 in main (argc=4, argv=0x7ffc2c1776c8, envp=0x7ffc2c1776f0) at /data/mercurial/mozilla-central/browser/app/nsBrowserApp.cpp:392


* 2. Started parsing css and parsing unit-less number as pixel since css::loader was in eCompatibility_NavQuirks mode [2].

[2] https://dxr.mozilla.org/mozilla-central/rev/fe895421dfbe1f1f8f1fc6a39bb20774423a6d74/layout/style/nsCSSParser.cpp#7842

Breakpoint 1, (anonymous namespace)::CSSParserImpl::ParseVariant (this=0x7f848b51c000, aValue=..., aVariantMask=67305478, aKeywordTable=0x0) at /data/mercurial/mozilla-central/layout/style/nsCSSParser.cpp:7842
7842          aValue.SetFloatValue(tk->mNumber, eCSSUnit_Pixel);
(gdb) bt
#0  (anonymous namespace)::CSSParserImpl::ParseVariant (this=0x7f848b51c000, aValue=..., aVariantMask=67305478, aKeywordTable=0x0) at /data/mercurial/mozilla-central/layout/style/nsCSSParser.cpp:7842
#1  0x00007f84a5f96e01 in (anonymous namespace)::CSSParserImpl::ParseVariantWithRestrictions (this=0x7f848b51c000, aValue=..., aVariantMask=67305478, aKeywordTable=0x0, aRestrictions=0)
    at /data/mercurial/mozilla-central/layout/style/nsCSSParser.cpp:7611
#2  0x00007f84a5fa266c in (anonymous namespace)::CSSParserImpl::ParseSingleValueProperty (this=0x7f848b51c000, aValue=..., aPropID=eCSSProperty_margin_left) at /data/mercurial/mozilla-central/layout/style/nsCSSParser.cpp:11838
#3  0x00007f84a5fa0f05 in (anonymous namespace)::CSSParserImpl::ParseProperty (this=0x7f848b51c000, aPropID=eCSSProperty_margin_left) at /data/mercurial/mozilla-central/layout/style/nsCSSParser.cpp:11303
#4  0x00007f84a5f96575 in (anonymous namespace)::CSSParserImpl::ParseDeclaration (this=0x7f848b51c000, aDeclaration=0x7f847b6da5c0, aFlags=3, aMustCallValueAppended=true, aChanged=0x7ffc2c1754ce, 
    aContext=(anonymous namespace)::CSSParserImpl::eCSSContext_General) at /data/mercurial/mozilla-central/layout/style/nsCSSParser.cpp:7334
#5  0x00007f84a5f94122 in (anonymous namespace)::CSSParserImpl::ParseDeclarationBlock (this=0x7f848b51c000, aFlags=3, aContext=(anonymous namespace)::CSSParserImpl::eCSSContext_General)
    at /data/mercurial/mozilla-central/layout/style/nsCSSParser.cpp:6599
#6  0x00007f84a5f90725 in (anonymous namespace)::CSSParserImpl::ParseRuleSet (this=0x7f848b51c000, aAppendFunc=0x7f84a5f83b00 <(anonymous namespace)::AppendRuleToSheet(mozilla::css::Rule*, void*)>, aData=0x7f848b51c000, 
    aInsideBraces=false) at /data/mercurial/mozilla-central/layout/style/nsCSSParser.cpp:5351
#7  0x00007f84a5f847cc in (anonymous namespace)::CSSParserImpl::ParseSheet (this=0x7f848b51c000, aInput=..., aSheetURI=0x7f847b3ffa00, aBaseURI=0x7f847b3ffa00, aSheetPrincipal=0x7f8476019a60, aLineNumber=1, aReusableSheets=0x0)
    at /data/mercurial/mozilla-central/layout/style/nsCSSParser.cpp:1728
#8  0x00007f84a5fb282b in nsCSSParser::ParseSheet (this=0x7ffc2c1757e0, aInput=..., aSheetURI=0x7f847b3ffa00, aBaseURI=0x7f847b3ffa00, aSheetPrincipal=0x7f8476019a60, aLineNumber=1, aReusableSheets=0x0)
    at /data/mercurial/mozilla-central/layout/style/nsCSSParser.cpp:17712
#9  0x00007f84a5f3f0f5 in mozilla::css::Loader::ParseSheet (this=0x7f847c21c680, aInput=..., aLoadData=0x7f8475bcd780, aCompleted=@0x7ffc2c175a90: false) at /data/mercurial/mozilla-central/layout/style/Loader.cpp:1762
#10 0x00007f84a5f3b7d3 in mozilla::css::SheetLoadData::OnStreamComplete (this=0x7f8475bcd780, aLoader=0x7f847c7911d0, aContext=0x0, aStatus=NS_OK, aBuffer=...) at /data/mercurial/mozilla-central/layout/style/Loader.cpp:979
#11 0x00007f84a244a600 in nsUnicharStreamLoader::OnStopRequest (this=0x7f847c7911d0, aRequest=0x7f8478ee2840, aContext=0x0, aStatus=NS_OK) at /data/mercurial/mozilla-central/netwerk/base/nsUnicharStreamLoader.cpp:97
#12 0x00007f84a23aa4eb in nsBaseChannel::OnStopRequest (this=0x7f8478ee27f0, request=0x7f8475e462c0, ctxt=0x0, status=NS_OK) at /data/mercurial/mozilla-central/netwerk/base/nsBaseChannel.cpp:826
#13 0x00007f84a23d2621 in nsInputStreamPump::OnStateStop (this=0x7f8475e462c0) at /data/mercurial/mozilla-central/netwerk/base/nsInputStreamPump.cpp:714
#14 0x00007f84a23d172d in nsInputStreamPump::OnInputStreamReady (this=0x7f8475e462c0, stream=0x7f8478a6dce0) at /data/mercurial/mozilla-central/netwerk/base/nsInputStreamPump.cpp:433
#15 0x00007f84a22796eb in nsInputStreamReadyEvent::Run (this=0x7f8479e431c0) at /data/mercurial/mozilla-central/xpcom/io/nsStreamUtils.cpp:95
#16 0x00007f84a22a1aa7 in nsThread::ProcessNextEvent (this=0x7f84b3b8c300, aMayWait=false, aResult=0x7ffc2c175edf) at /data/mercurial/mozilla-central/xpcom/threads/nsThread.cpp:1058
#17 0x00007f84a230a701 in NS_ProcessNextEvent (aThread=0x7f84b3b8c300, aMayWait=false) at /data/mercurial/mozilla-central/xpcom/glue/nsThreadUtils.cpp:290
#18 0x00007f84a2a5f327 in mozilla::ipc::MessagePump::Run (this=0x7f849ed99800, aDelegate=0x7f84b3b55880) at /data/mercurial/mozilla-central/ipc/glue/MessagePump.cpp:96
#19 0x00007f84a29cf905 in MessageLoop::RunInternal (this=0x7f84b3b55880) at /data/mercurial/mozilla-central/ipc/chromium/src/base/message_loop.cc:232
#20 0x00007f84a29cf89a in MessageLoop::RunHandler (this=0x7f84b3b55880) at /data/mercurial/mozilla-central/ipc/chromium/src/base/message_loop.cc:225
#21 0x00007f84a29cf873 in MessageLoop::Run (this=0x7f84b3b55880) at /data/mercurial/mozilla-central/ipc/chromium/src/base/message_loop.cc:205
#22 0x00007f84a5d34d18 in nsBaseAppShell::Run (this=0x7f8491715c50) at /data/mercurial/mozilla-central/widget/nsBaseAppShell.cpp:156
#23 0x00007f84a6ce6353 in nsAppStartup::Run (this=0x7f84917181f0) at /data/mercurial/mozilla-central/toolkit/components/startup/nsAppStartup.cpp:284
#24 0x00007f84a6da5d0c in XREMain::XRE_mainRun (this=0x7ffc2c1762d0) at /data/mercurial/mozilla-central/toolkit/xre/nsAppRunner.cpp:4302
#25 0x00007f84a6da62a1 in XREMain::XRE_main (this=0x7ffc2c1762d0, argc=4, argv=0x7ffc2c1776c8, aAppData=0x7ffc2c1764f0) at /data/mercurial/mozilla-central/toolkit/xre/nsAppRunner.cpp:4429
#26 0x00007f84a6da6571 in XRE_main (argc=4, argv=0x7ffc2c1776c8, aAppData=0x7ffc2c1764f0, aFlags=0) at /data/mercurial/mozilla-central/toolkit/xre/nsAppRunner.cpp:4520
#27 0x0000000000405a71 in do_main (argc=4, argv=0x7ffc2c1776c8, envp=0x7ffc2c1776f0, xreDirectory=0x7f84b3b6b780) at /data/mercurial/mozilla-central/browser/app/nsBrowserApp.cpp:259
#28 0x0000000000405dd8 in main (argc=4, argv=0x7ffc2c1776c8, envp=0x7ffc2c1776f0) at /data/mercurial/mozilla-central/browser/app/nsBrowserApp.cpp:392


* 3. FlushTimer timed out to continue parsing and set HTMLDocument to eCompatibility_FullStandards mode.

Breakpoint 2, mozilla::css::Loader::SetCompatibilityMode (this=0x7f847c21c680, aCompatMode=eCompatibility_FullStandards) at /data/mercurial/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/css/Loader.h:207
207       { mCompatMode = aCompatMode; }
(gdb) bt
#0  mozilla::css::Loader::SetCompatibilityMode (this=0x7f847c21c680, aCompatMode=eCompatibility_FullStandards) at /data/mercurial/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/css/Loader.h:207
#1  0x00007f84a5194d98 in nsHTMLDocument::SetCompatibilityMode (this=0x7f847c7d3000, aMode=eCompatibility_FullStandards) at /data/mercurial/mozilla-central/dom/html/nsHTMLDocument.cpp:856
#2  0x00007f84a3653100 in nsHtml5DocumentBuilder::SetDocumentMode (this=0x7f847c608800, m=STANDARDS_MODE) at /data/mercurial/mozilla-central/parser/html/nsHtml5DocumentBuilder.cpp:105
#3  0x00007f84a36a851f in nsHtml5TreeOperation::Perform (this=0x7f847e3ba408, aBuilder=0x7f847c608800, aScriptElement=0x7ffc2c175c50) at /data/mercurial/mozilla-central/parser/html/nsHtml5TreeOperation.cpp:677
#4  0x00007f84a368e41e in nsHtml5TreeOpExecutor::RunFlushLoop (this=0x7f847c608800) at /data/mercurial/mozilla-central/parser/html/nsHtml5TreeOpExecutor.cpp:448
#5  0x00007f84a368dbfc in FlushTimerCallback (aTimer=0x7f847c791390, aClosure=0x0) at /data/mercurial/mozilla-central/parser/html/nsHtml5TreeOpExecutor.cpp:247
#6  0x00007f84a22c2ec8 in nsTimerImpl::Fire (this=0x7f847c791390) at /data/mercurial/mozilla-central/xpcom/threads/nsTimerImpl.cpp:521
#7  0x00007f84a229bb67 in nsTimerEvent::Run (this=0x7f848ce9a4b8) at /data/mercurial/mozilla-central/xpcom/threads/TimerThread.cpp:286
#8  0x00007f84a22a1aa7 in nsThread::ProcessNextEvent (this=0x7f84b3b8c300, aMayWait=false, aResult=0x7ffc2c175edf) at /data/mercurial/mozilla-central/xpcom/threads/nsThread.cpp:1058
#9  0x00007f84a230a701 in NS_ProcessNextEvent (aThread=0x7f84b3b8c300, aMayWait=false) at /data/mercurial/mozilla-central/xpcom/glue/nsThreadUtils.cpp:290
#10 0x00007f84a2a5f327 in mozilla::ipc::MessagePump::Run (this=0x7f849ed99800, aDelegate=0x7f84b3b55880) at /data/mercurial/mozilla-central/ipc/glue/MessagePump.cpp:96
#11 0x00007f84a29cf905 in MessageLoop::RunInternal (this=0x7f84b3b55880) at /data/mercurial/mozilla-central/ipc/chromium/src/base/message_loop.cc:232
#12 0x00007f84a29cf89a in MessageLoop::RunHandler (this=0x7f84b3b55880) at /data/mercurial/mozilla-central/ipc/chromium/src/base/message_loop.cc:225
#13 0x00007f84a29cf873 in MessageLoop::Run (this=0x7f84b3b55880) at /data/mercurial/mozilla-central/ipc/chromium/src/base/message_loop.cc:205
#14 0x00007f84a5d34d18 in nsBaseAppShell::Run (this=0x7f8491715c50) at /data/mercurial/mozilla-central/widget/nsBaseAppShell.cpp:156
#15 0x00007f84a6ce6353 in nsAppStartup::Run (this=0x7f84917181f0) at /data/mercurial/mozilla-central/toolkit/components/startup/nsAppStartup.cpp:284
#16 0x00007f84a6da5d0c in XREMain::XRE_mainRun (this=0x7ffc2c1762d0) at /data/mercurial/mozilla-central/toolkit/xre/nsAppRunner.cpp:4302
#17 0x00007f84a6da62a1 in XREMain::XRE_main (this=0x7ffc2c1762d0, argc=4, argv=0x7ffc2c1776c8, aAppData=0x7ffc2c1764f0) at /data/mercurial/mozilla-central/toolkit/xre/nsAppRunner.cpp:4429
#18 0x00007f84a6da6571 in XRE_main (argc=4, argv=0x7ffc2c1776c8, aAppData=0x7ffc2c1764f0, aFlags=0) at /data/mercurial/mozilla-central/toolkit/xre/nsAppRunner.cpp:4520
#19 0x0000000000405a71 in do_main (argc=4, argv=0x7ffc2c1776c8, envp=0x7ffc2c1776f0, xreDirectory=0x7f84b3b6b780) at /data/mercurial/mozilla-central/browser/app/nsBrowserApp.cpp:259
#20 0x0000000000405dd8 in main (argc=4, argv=0x7ffc2c1776c8, envp=0x7ffc2c1776f0) at /data/mercurial/mozilla-central/browser/app/nsBrowserApp.cpp:392
> It is because css::Loader is in "eCompatibility_NavQuirks" mode during parsing.

Ah, yeah that explains the difference between.

(non-quirks)
data:text/html,<!DOCTYPE html><style>*{margin:0}div{width:100px;height:100px;background:green;position:absolute;}.fail {background:red;top:-100px;}</style><div class=pass>PASS</div><div class=fail style="margin-top:100">FAIL</div>

and 

(quirks)
data:text/html,<style>*{margin:0}div{width:100px;height:100px;background:green;position:absolute;}.fail {background:red;top:-100px;}</style><div class=pass>PASS</div><div class=fail style="margin-top:100">FAIL</div>

https://quirks.spec.whatwg.org/#the-unitless-length-quirk
Tracking 51+ - agree with Comment 7 that this is worthwhile to track.
If we have a simple fix for this that we can verify I would still take a patch in beta. It won't block release though.
(In reply to Gerry Chang [:gchang] from comment #13)
> Hi HsinYi,
> Can you help to find someone to investigate this? Not sure if this is
> related to DOM.

Comment 15 seems point out the root cause, but I guess we still need Olli's input here.
Flags: needinfo?(htsai)
yup, I'll try to look at this during this week.
Is the issue that we start doing CSS prefetches before we've performed the "set the doctype" bit, effectively?

Shouldn't we be throttling those too, if we throttle the overall parsing process?
yes. Though, I'm trying to understand what guarantees this couldn't happen for foreground tabs too.
Reading some code.
One could argue that background tabs shouldn't do speculative loading at all, since all the possible processing power and network bandwidth should be given to the foreground tab (well, at least up to some level)
So, something like this is one option to fix the issue for background tabs, but I still haven't convinced myself why the issue couldn't happen in foreground too. But that could be a separate bug.

However, I'm not quite sure about this approach. If foreground tab has been loaded, shouldn't background tab just do all the speculative loading normally.
But then, these days web pages do all sort of loading all the time, so one never knows whether foreground tab isn't going to do some more loading.


This is feedback-ish? review? :)
Flags: needinfo?(bugs)
Attachment #8784392 - Flags: review?(hsivonen)
Comment on attachment 8784392 [details] [diff] [review]
prevent_speculative_in_bg.diff

We have a race condition here even when parsing in the foreground tab. I'm a bit surprised that problem hasn't been discovered earlier.

To fix the problem properly, we need to transfer the quirkiness from the parser thread to the main thread via the speculation queue instead of the operation queue. That is, in https://dxr.mozilla.org/mozilla-central/source/parser/html/nsHtml5TreeBuilderCppSupplement.h?q=%2Bfunction%3AnsHtml5TreeBuilder%3A%3AdocumentMode%28nsHtml5DocumentMode%29&redirect_type=single#1220 we should be using the pattern seen at https://dxr.mozilla.org/mozilla-central/source/parser/html/nsHtml5TreeBuilderCppSupplement.h?q=%2Bfunction%3A%22nsHtml5TreeBuilder%3A%3ASetDocumentCharset%28nsACString_internal+%26%2C+int32_t%29%22&redirect_type=single#1074 .

This patch is r- specifically because the pattern seen via the second link already exists: When the speculation queue is present, the correctness of the results depends on stuff that are not speculative loads but that have to be transferred via the speculative load queue being acted upon. While it would be OK to drop the speculative load operations in a background tab, it's not OK to drop everything that travels in that queue.

If we don't want background tabs to do speculation, it would be useful to turn off the speculative parse, but that requires me to find the time to figure out what's wrong with bug 1151048.
Flags: needinfo?(hsivonen)
Attachment #8784392 - Flags: review?(hsivonen) → review-
Assignee: nobody → hsivonen
whether or not to turn off speculative parsing in background tabs could be handled in a different bug, if this bug just deals with the document mode issue.

Thanks for looking into this.
See Also: → 1298764
(In reply to Olli Pettay [:smaug] (vacation Aug 25-28) from comment #26)
> whether or not to turn off speculative parsing in background tabs could be
> handled in a different bug

Filed bug 1298764.
Comment on attachment 8784802 [details]
Bug 1293252 - Transfer document quirkiness via the speculative load queue.

https://reviewboard.mozilla.org/r/74122/#review73410

::: parser/html/nsHtml5SpeculativeLoad.h:224
(Diff revision 1)
>       * eSpeculativeLoadSetDocumentCharset it is the charset that the
>       * document's charset is being set to. Otherwise it's empty.
>       */
>      nsString mCharset;
>      /**
>       * If mOpCode is eSpeculativeLoadSetDocumentCharset, this is a

Update comment to "If mOpCode is eSpeculativeLoadSetDocumentCharset or eSpeculativeLoadSetDocumentMode"
Comment on attachment 8784802 [details]
Bug 1293252 - Transfer document quirkiness via the speculative load queue.

https://reviewboard.mozilla.org/r/74122/#review73420
Attachment #8784802 - Flags: review?(wchen) → review+
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fe7ad4207469
Transfer document quirkiness via the speculative load queue. r=wchen
https://hg.mozilla.org/mozilla-central/rev/fe7ad4207469
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Hello there, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(ajfhajf)
Hello Henri, should we uplift this fix to Beta50?
Flags: needinfo?(hsivonen)
(In reply to Ritu Kothari (:ritu) from comment #34)
> Hello there, could you please verify this issue is fixed as expected on a
> latest Nightly build? Thanks!
Yes, this issue is fixed in the latest Nightly on my machine!
(In reply to Ritu Kothari (:ritu) from comment #35)
> Hello Henri, should we uplift this fix to Beta50?

I think we should. The patch applies as-is, has been tested on 51 and the bug causes badness that's potentially hard for Web devs to track down or deal with.
Flags: needinfo?(hsivonen)
Status: RESOLVED → VERIFIED
(In reply to Henri Sivonen (:hsivonen) from comment #37)
> (In reply to Ritu Kothari (:ritu) from comment #35)
> > Hello Henri, should we uplift this fix to Beta50?
> 
> I think we should. The patch applies as-is, has been tested on 51 and the
> bug causes badness that's potentially hard for Web devs to track down or
> deal with.

Could you please nominate a patch for uplift to Beta50? Thanks!
Flags: needinfo?(hsivonen)
Comment on attachment 8784802 [details]
Bug 1293252 - Transfer document quirkiness via the speculative load queue.

Approval Request Comment
> [Feature/regressing bug #]:

Bug 734015.

> [User impact if declined]:

Occasionally documents that should get Standards Mode CSS parsing get Quirks Mode CSS parsing. This may lead to pages looking wrong. Possibly there could be other mode mixups.

> [Describe test coverage new/current, TreeHerder]:

None. The nature of the bug makes it hard to write automated tests for it.

However, HTML and CSS parsing in general have a lot of test coverage, so while there aren't tests showing that this bug has been fixed, there are plenty of tests that increase confidence in this not having caused adjacent regressions.

> [Risks and why]: 

Fairly low. The patch is well understood, relatively simple, applies cleanly to beta and has baked on Nightly and Aurora for a while.

> [String/UUID change made/needed]:

None.
Flags: needinfo?(hsivonen)
Attachment #8784802 - Flags: approval-mozilla-beta?
Comment on attachment 8784802 [details]
Bug 1293252 - Transfer document quirkiness via the speculative load queue.

Fix was verified on 51, let's uplift to Beta50.
Attachment #8784802 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(ajfhajf)
You need to log in before you can comment on or make changes to this bug.