293825 - [FIX]Improve CSSLoader api

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

I started poking at bug 84582 again and realized that making the CSSLoader api
saner was more or less a prerequisite for me fixing that bug...

Patch coming up; the goal is to change behavior as little as possible, but at
the same time make async loads really async, make it clear when observers are
notified, make error handling more consistent, and move the alternate sheet
stuff as much as possible into the loader.

Note that this patch depends on the patch in bug 293818.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch (diff -w)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch without -w

Comment 3

[Boris Zbarsky [:bzbarsky]]

For now, because of the way the parser-blocking stuff works, I addded the
following in the "sheet already complete" branch in LoadStyleLink before
PostLoadEvent:

    // XXXbz nasty hack because we end up blocking the parser in XUL, then
    // unblocking it, but firing onload first... Just don't block the parser
    // for stylesheets that are already complete.  It's not like the load event
    // will do anything useful for us anyway here.
    *aIsAlternate = PR_TRUE;

Otherwise any XUL doc with more than one sheet was having issues (since the
loadgroup counter got decremented before the new sheet was seen by the content
sink, basically).

The other option would be to make UnblockOnload async, I guess... Not sure
whether we want that.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Ignore comment 3.  That was happening because I was unblocking onload in both
HandleEvent and DestroyEvent.  The former should not have been there, and
removing it makes things here quite happy without that hack.

Comment 5

[Boris Zbarsky [:bzbarsky]]

It looks like the problems described in bug 188030 are not around anymore; XBL
now always does a sync sheet load for chrome...  Given that, I'd rather not
design the CSSLoader api around XBL brokenness (and would like the fully-async
API for the async case).

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 183325 [details] [diff] [review]
Proposed patch (diff -w)

> Index: layout/style/nsICSSLoaderObserver.h
> ===================================================================

> +   * @param aWasAlternate whether the sheet was an alternate.  This will always
> +   *        match the value LoadStyleLink or LoadInlineStyle returned if one of
> +   *        those methods were used to load the sheet, and will always be false
> +   *        otherwise.

Er, those return nsresult, you mean "... LoadStyleLink or LoadInlineStyle
returned in aIsAlternate ...". Isn't it simpler to just say that it returns
"whether the stylesheet ended up being an alternate sheet."?

>Index: layout/style/nsICSSLoader.h
>===================================================================

>+   * @return whether the stylesheet ended up being an alternate sheet.

This seems to describe aIsAlternate.

>   NS_IMETHOD LoadInlineStyle(nsIContent* aElement,


>+   * @return whether the stylesheet actually ended up being an alternate sheet.
>+   *         Note that this need not match aHasAlternateRel.

This seems to describe aIsAlternate. It always matches if aHasAlternateRel is
true, right?

>   NS_IMETHOD LoadStyleLink(nsIContent* aElement,

>+  /**
>+   * Load a child (@import-ed) style sheet.  In addition to loading the sheet,
>+   * this method will insert it into the child sheet list of aParentSheet.  If
>+   * there is no sheet currently being parsed, aParentSheet will be QIed to
>+   * nsICSSLoaderObserver and asynchronously notified, just like for
>+   * LoadStyleLink, when the child sheet is complete if it's not already.  If
>+   * the child sheet _is_ already complete, no notification will be sent.

I find this sentence confusing. "... just like for LoadStyleLink, once the
child sheet is complete. If ..."?

>+   * @return the loaded, complete sheet.

This seems to describe aSheet.

>+  NS_IMETHOD LoadSheetSync(nsIURI* aURL, nsICSSStyleSheet** aSheet) = 0;

>Index: layout/style/nsCSSLoader.h
>===================================================================

>+  // mIsAgent is true if the load was triggered by LoadSheetSync or
>+  // LoadSheet or an @import from such a sheet.  Agent loads can

Hmm, I didn't see any doc for LoadSheetSync or LoadSheet that said they load
agent sheets.

>+  // proceed even if we have no document.
>+  PRPackedBool               mIsAgent : 1;

>   // mIsLoading is true from the moment we are placed in the loader's
>   // "loading datas" table (right after the async channel is opened)
>   // to the moment we are removed from said table (due to the load
>   // completing or being cancelled).
>-  PRPackedBool               mIsLoading; // Set once the data is in the "loading" table
>+  PRPackedBool               mIsLoading : 1; // Set once the data is in the "loading" table

While you're in there, remove the redundant comment?

>Index: layout/style/nsCSSLoader.cpp
>===================================================================

> PR_STATIC_CALLBACK(PLDHashOperator)
> StartNonAlternates(nsIURI *aKey, SheetLoadData* &aData, void* aClosure)

>+  // Note that we don't want to affect what the selected style set is,
>+  // so use PR_TRUE for aHasAlternateRel.

It seems a bit fragile that aHasAlternateRel affects what IsAlternate does to
the selected style set.

>+CSSLoaderImpl::IsAlternate(const nsAString& aTitle, PRBool aHasAlternateRel)

>+  if (!aHasAlternateRel && mDocument && mPreferredSheet.IsEmpty()) {
>+    // There's no preferred set yet, and we now have a sheet with a title.
>+    // Make that be the preferred set.
>+    // XXXbz maybe this should be checking IsVoid(), actually, since the
>+    // preferred set can be explicitly set to the empty string.  Look into
>+    // this.

IMHO, yes. File a bug?

>+    mDocument->SetHeaderData(nsHTMLAtoms::headerDefaultStyle, aTitle);

Please document clearly that IsAlternate can set the default styleset.

>+nsresult
>+CSSLoaderImpl::PostLoadEvent(nsIURI* aURI,
>+                             nsICSSStyleSheet* aSheet,
>+                             nsICSSLoaderObserver* aObserver,
>+                             nsIParser* aParserToUnblock,
>+                             PRBool aWasAlternate)

>+  nsresult rv;
>+  nsCOMPtr<nsIEventQueueService> eventQService =
>+    do_GetService("@mozilla.org/event-queue-service;1", &rv);
>+  NS_ENSURE_TRUE(eventQService, rv);

nsContentUtils::EventQueueService().

>+void
>+CSSLoaderImpl::HandleLoadEvent(SheetLoadData* aEvent)

>+  if (mDocument) {
>+    mDocument->UnblockOnload();
>+  }
>+}
>+
>+void
>+CSSLoaderImpl::DestroyLoadEvent(SheetLoadData* aEvent)
>+{
>+  mPostedEvents.RemoveElement(aEvent);
>+  NS_RELEASE(aEvent);
>+  if (mDocument) {
>+    mDocument->UnblockOnload();

Won't this call UnblockOnload too many times (since you called it in
HandleLoadEvent)?

>Index: content/xul/document/src/nsXULContentSink.cpp
>===================================================================

>-        // Nope, we need to load it asynchronously
>-        PRBool blockParser = PR_FALSE;
>-        if (! aAlternate) {
>-            if (!aTitle.IsEmpty()) {  // possibly preferred sheet
>-                if (mPreferredStyle.IsEmpty()) {
>-                    mPreferredStyle = aTitle;

I think this means you can remove mPreferredStyle.

Why do we pass aWasAlternate to StyleSheetLoaded?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Er, those return nsresult

I meant "return" in the idl sense...  For the rest, I just wanted to precisely
define what it means to have "ended up being an alternate sheet", since whether
a sheet is alternate can actually change while the sheet is loading.  I'll
explicitly mention aIsAlternate.

> >+   * @return whether the stylesheet ended up being an alternate sheet.
> This seems to describe aIsAlternate.

Again, that would be the return value if this were expressed in IDL...  I
suppose I can call it @param instead if you'd prefer.  Same for a few other places.

> It always matches if aHasAlternateRel is true, right?

No, it matches if the title isn't the default title.  All aHasAlternateRel
affects is setting of the default if no default is set yet.

> I find this sentence confusing.

What I'm trying to say is that if the child is not yet complete by the time
LoadChildSheet returns, when it becomes complete the parent will be notified. 
I'll rephrase to make this clearer.

> Hmm, I didn't see any doc for LoadSheetSync or LoadSheet that said they load
> agent sheets.

mIsAgent is perhaps somewhat misnamed (just as LoadAgentSheet was); I could
rename it to mNonDocumentSheet throughout, I suppose.  Let me know if you want
me to do that.

> It seems a bit fragile that aHasAlternateRel affects what IsAlternate does to
> the selected style set

If there is no currently selected style set then per spec

  <link rel="stylesheet" title="foo">

should make "foo" the currently selected set and make this sheet not be an
alternate sheet, while

  <link rel="alternate stylesheet" title="foo">

should leave things as they are (no selected set) and make this sheet be an
alternate sheet.

I could move the changing of the style set out of IsAlternate and just call that
function before calling IsAlternate in every single place where I call
IsAlternate...  But that seemed a little silly.

> IMHO, yes. File a bug?

It sorta falls under the bug about scriptable selection of style sets I already
have...

> Please document clearly that IsAlternate can set the default styleset.

Will do.

> nsContentUtils::EventQueueService().

I should have posted an updated patch.  :(

> Won't this call UnblockOnload too many times

See comment 4.  I really should have posted an updated patch.  :(

> I think this means you can remove mPreferredStyle.

Hmm..  Actually, I can, but I need to make sure to copy over the header from the
proto doc to the non-proto one.  I'll do that; could you check over that part
carefully again when I post the updated patch?

> Why do we pass aWasAlternate to StyleSheetLoaded?

Because I need that for bug 84582.

Comment 8

[Hixie (not reading bugmail)]

peterv: In case this helps -- there are three types of stylesheet sets;
persistent, preferred, and alternate. The currently selected stylesheet set is
one of the preferred and alternate sets. The persistent stylesheets are enabled
regardless of which set is selected. Hope this helps. :-)

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 183325 [details] [diff] [review]
Proposed patch (diff -w)

Setting to minus, awaiting new patch.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated diff -w

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Same without the -w

Comment 12

[Peter Van der Beken [:peterv]]

Comment on attachment 195464 [details] [diff] [review]
Updated diff -w

With adding a comment that LoadSheet(Sync) load non-document stylesheets.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Hurray!  Fixed!

Comment 14

[Johnny Stenback (:jst)]

Well, yes, but it seems like this caused random crashes in odd places... I
pointed Purify at this, and here's what I got:

    [E] FMW: Free memory write in CSSLoaderImpl::LoadChildSheet(nsICSSStyleSheet
*,nsIURI *,nsMediaList *,nsICSSImportRule *) {2 occurrences}
        Writing 1 byte to 0x05987ba0 (1 byte at 0x05987ba0 illegal)
        Address 0x05987ba0 is 112 bytes into a 124 byte block at 0x05987b30
        Address 0x05987ba0 points to a C++ new block in heap 0x01c70000
        Thread ID: 0xecc
        Error location
        CSSLoaderImpl::LoadChildSheet(nsICSSStyleSheet *,nsIURI *,nsMediaList
*,nsICSSImportRule *) [e:\tip\mozilla\layout\style\nscssloader.cpp:1839]
          rv = LoadSheet(data, state);
          NS_ENSURE_SUCCESS(rv, rv);
        
     =>   data->mMustNotify = PR_TRUE;
          return rv;
        }
        
        CSSParserImpl::ProcessImport(UINT&,nsString const&,nsMediaList
*,(*)(nsICSSRule *,void *),void *)
[e:\tip\mozilla\layout\style\nscssparser.cpp:1365]
        CSSParserImpl::ParseImportRule(UINT&,(*)(nsICSSRule *,void *),void *)
[e:\tip\mozilla\layout\style\nscssparser.cpp:1336]
        CSSParserImpl::ParseAtRule(UINT&,(*)(nsICSSRule *,void *),void *)
[e:\tip\mozilla\layout\style\nscssparser.cpp:1177]
        CSSParserImpl::Parse(nsIUnicharInputStream *,nsIURI *,nsIURI
*,UINT,nsICSSStyleSheet *&) [e:\tip\mozilla\layout\style\nscssparser.cpp:697]
        CSSLoaderImpl::ParseSheet(nsIUnicharInputStream *,SheetLoadData *,int&)
[e:\tip\mozilla\layout\style\nscssloader.cpp:1421]
        CSSLoaderImpl::LoadSheet(SheetLoadData *,StyleSheetState)
[e:\tip\mozilla\layout\style\nscssloader.cpp:1272]
        CSSLoaderImpl::LoadChildSheet(nsICSSStyleSheet *,nsIURI *,nsMediaList
*,nsICSSImportRule *) [e:\tip\mozilla\layout\style\nscssloader.cpp:1836]
        CSSParserImpl::ProcessImport(UINT&,nsString const&,nsMediaList
*,(*)(nsICSSRule *,void *),void *)
[e:\tip\mozilla\layout\style\nscssparser.cpp:1365]
        CSSParserImpl::ParseImportRule(UINT&,(*)(nsICSSRule *,void *),void *)
[e:\tip\mozilla\layout\style\nscssparser.cpp:1336]
    Allocation location
        new(UINT)      [f:\vs70builds\3077\vc\crtbld\crt\src\newop.cpp:10]
        CSSLoaderImpl::LoadChildSheet(nsICSSStyleSheet *,nsIURI *,nsMediaList
*,nsICSSImportRule *) [e:\tip\mozilla\layout\style\nscssloader.cpp:1825]
        CSSParserImpl::ProcessImport(UINT&,nsString const&,nsMediaList
*,(*)(nsICSSRule *,void *),void *)
[e:\tip\mozilla\layout\style\nscssparser.cpp:1365]
        CSSParserImpl::ParseImportRule(UINT&,(*)(nsICSSRule *,void *),void *)
[e:\tip\mozilla\layout\style\nscssparser.cpp:1336]
        CSSParserImpl::ParseAtRule(UINT&,(*)(nsICSSRule *,void *),void *)
[e:\tip\mozilla\layout\style\nscssparser.cpp:1177]
        CSSParserImpl::Parse(nsIUnicharInputStream *,nsIURI *,nsIURI
*,UINT,nsICSSStyleSheet *&) [e:\tip\mozilla\layout\style\nscssparser.cpp:697]
        CSSLoaderImpl::ParseSheet(nsIUnicharInputStream *,SheetLoadData *,int&)
[e:\tip\mozilla\layout\style\nscssloader.cpp:1421]
        CSSLoaderImpl::LoadSheet(SheetLoadData *,StyleSheetState)
[e:\tip\mozilla\layout\style\nscssloader.cpp:1272]
        CSSLoaderImpl::LoadChildSheet(nsICSSStyleSheet *,nsIURI *,nsMediaList
*,nsICSSImportRule *) [e:\tip\mozilla\layout\style\nscssloader.cpp:1836]
        CSSParserImpl::ProcessImport(UINT&,nsString const&,nsMediaList
*,(*)(nsICSSRule *,void *),void *)
[e:\tip\mozilla\layout\style\nscssparser.cpp:1365]
    Free location
        SheetLoadData::`vector deleting destructor'(UINT)
[E:\tip\fb-prf\dist\bin\components\gklayout.dll]
        SheetLoadData::Release(void)
[e:\tip\mozilla\layout\style\nscssloader.cpp:141]
        CSSLoaderImpl::SheetComplete(SheetLoadData *,UINT)
[e:\tip\mozilla\layout\style\nscssloader.cpp:1551]
        CSSLoaderImpl::ParseSheet(nsIUnicharInputStream *,SheetLoadData *,int&)
[e:\tip\mozilla\layout\style\nscssloader.cpp:1431]
        CSSLoaderImpl::LoadSheet(SheetLoadData *,StyleSheetState)
[e:\tip\mozilla\layout\style\nscssloader.cpp:1272]
        CSSLoaderImpl::LoadChildSheet(nsICSSStyleSheet *,nsIURI *,nsMediaList
*,nsICSSImportRule *) [e:\tip\mozilla\layout\style\nscssloader.cpp:1836]
        CSSParserImpl::ProcessImport(UINT&,nsString const&,nsMediaList
*,(*)(nsICSSRule *,void *),void *)
[e:\tip\mozilla\layout\style\nscssparser.cpp:1365]
        CSSParserImpl::ParseImportRule(UINT&,(*)(nsICSSRule *,void *),void *)
[e:\tip\mozilla\layout\style\nscssparser.cpp:1336]
        CSSParserImpl::ParseAtRule(UINT&,(*)(nsICSSRule *,void *),void *)
[e:\tip\mozilla\layout\style\nscssparser.cpp:1177]
        CSSParserImpl::Parse(nsIUnicharInputStream *,nsIURI *,nsIURI
*,UINT,nsICSSStyleSheet *&) [e:\tip\mozilla\layout\style\nscssparser.cpp:697]

I'll try to have a look at this, but I don't know when I'll get to it. Reopening
until we figure this out.

Comment 15

[Johnny Stenback (:jst)]

Attachment: Fix the FMW found by Purify.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Hmm....  jst, does saving data->mSyncLoad in a local before calling LoadSheet
and only messing with data->mMustNotify if data->mSyncLoad is false also fix the
FMR?  That's the only way I can think of that |data| would be gone there; if
|data| is dying and |data->mSyncLoad| is false and NS_OK is being returned from
LoadSheet, we have a problem.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 196284 [details] [diff] [review]
Like so

Yeah, that fixes it too. It'd still be cool to eliminate some of this manual
refcounting in favor of nsRefPtr<>'s and a cleaner model in general, but this
does at least fix the crashes I'm seeing, and the FMW in Purify.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Checked that patch in.