Status

()

defect
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: zwol, Assigned: zwol)

Tracking

(Blocks 2 bugs)

Trunk
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

Assignee

Description

9 years ago
Posted patch patch v1 (obsolete) — Splinter Review
The attached patch deCOMtaminates and devirtualizes ns(I)CSSLoader.  There is now a plain class called mozilla::CSS::Loader, declared in nsCSSLoader.h, which has almost exactly the same interfaces as the old nsICSSLoader.  (I made a few always-returns-NS_OK methods be void-return-value, and changed GetEnabled() to return a boolean rather than writing to an out-parameter.)  There are a very few places where we need to hold onto a loader when its owning document might be destroyed, so it is still reference-counted.

nsICSSLoaderObserver is not changed.

This patch causes link errors below parser/ in a --disable-libxul configuration.  I don't know how to fix that.  Also, it might be that I should sprinkle NS_HIDDEN around nsCSSLoader.h, but I don't really understand the rules for that.

There is a structural dependency on the deCOM patch in bug 523496 (but not the other patch, which is getting split to its own bug and shuffled later in this overall patch series, anyway).
Attachment #425082 - Flags: superreview?(dbaron)
Attachment #425082 - Flags: review?(bzbarsky)
Assignee

Comment 1

9 years ago
I forgot to mention that there were a couple of dusty corners where we were calling do_CreateInstance(nsICSSLoader) with a comment saying that the loader should really be retrieved from the document in this case.  They all had a document readily available, so I changed those to retrieve the loader from the document.  I *think* the only possible negative consequence of this is that some XUL or UA sheets might be processed in quirks mode when they shouldn't be.  If that's a problem, it would be relatively easy to add flags to the ->LoadSheet() and/or ->LoadSheetSync() methods to tell them to ignore the compatibility mode.
> I don't know how to fix that.

The obvious ways seem to be:

1)  Drop support for non-libxul builds.
2)  Make the API entry points inlines that either call a virtual method (if the
    caller is outside layout) or a non-virtual method.
3)  Move stylesheet preloads to a virtual nsIDocument API that calls into the
    document's CSSLoader.

> Also, it might be that I should sprinkle NS_HIDDEN

I believe at this point NS_HIDDEN is a no-op (and hence just noise).

Comment 3

9 years ago
4) move parser into gklayout

I obviously prefer #1, but we still have build work to do to make that faster.
Assignee

Comment 4

9 years ago
For purposes of this bug, I think (3) will be easiest.
Assignee

Updated

9 years ago
Blocks: deCOM
Assignee

Comment 5

9 years ago
This patch fixes the link failures that were caused by version 1 of the original patch.  It's intended to be committed ahead of the actual deCOM work.

Question for reviewers:

+nsDocument::PreloadStyle(nsIURI* uri, const nsAString& charset)
+{
+  // The CSSLoader will retain this object after we return.
+  nsCOMPtr<nsICSSLoaderObserver> obs = new StubCSSLoaderObserver();
+
+  // Is lossy conversion to ASCII really appropriate here?
+  CSSLoader()->LoadSheet(uri, NodePrincipal(),
+                         NS_LossyConvertUTF16toASCII(charset),
+                         obs);
+}

Could we avoid the nsCOMPtr here and just pass the result of "new StubCSSLoaderObserver()" directly to LoadSheet, or would that get the refcounting wrong (in either direction)?
Attachment #425082 - Attachment is obsolete: true
Attachment #428480 - Flags: superreview?(dbaron)
Attachment #428480 - Flags: review?(bzbarsky)
Attachment #425082 - Flags: superreview?(dbaron)
Attachment #425082 - Flags: review?(bzbarsky)
Assignee

Comment 6

9 years ago
This corresponds to the original patch, and has not substantively changed since then.  It is intended to apply after the ns(I)Document changes.
Attachment #428481 - Flags: superreview?(dbaron)
Attachment #428481 - Flags: review?(bzbarsky)
> Could we avoid the nsCOMPtr here

No.  At least not unless LoadSheet explicitly documents that you can and is careful to make that work.  That's a general rule for passing refcounted things, fwiw.

Consider a simple example that can go wrong if you just pass a refcounted object with a 0 initial refcount to some random callee:

  void func(nsIFoo* foo) 
  {
    if (something) {
      nsCOMPtr<nsIBar> bar = do_QueryInterface(foo);
      ...
    }
    // foo is now deleted
  }
Assignee

Comment 8

9 years ago
Good to know, thanks.   Do you think you'll have time to review the patches soon?
That's the plan, yes.  Aiming for today or tomorrow.
Comment on attachment 428480 [details] [diff] [review]
patch v2, part 1: go through nsDocument for speculative and chrome sheet loads

>+  // Is lossy conversion to ASCII really appropriate here?

Yes.  Charset names are ASCII.

r=bzbarsky
Attachment #428480 - Flags: review?(bzbarsky) → review+
Assignee

Comment 11

9 years ago
(In reply to comment #10)
> (From update of attachment 428480 [details] [diff] [review])
> >+  // Is lossy conversion to ASCII really appropriate here?
> 
> Yes.  Charset names are ASCII.

In my copy that comment now reads
 // Charset names are always ASCII
Comment on attachment 428481 [details] [diff] [review]
patch v2, part 2: deCOM nsCSSLoader

>+++ b/content/base/src/nsDocument.cpp
>+  mCSSLoader = new mozilla::CSS::Loader(this);

This works because the Loader constructor sets mRefCnt to 1, but I'd prefer it didn't do that (and that we just NS_ADDREF here).  That would be a lot less surprising for callers and hence less leak-prone.

>+++ b/layout/base/nsDocumentViewer.cpp
>+        nsRefPtr<mozilla::CSS::Loader>
>+          cssLoader(getter_AddRefs(new mozilla::CSS::Loader()));

And then this could just assign into the nsRefPtr.

>+++ b/layout/base/nsStyleSheetService.cpp
>+  nsRefPtr<mozilla::CSS::Loader>
>+    loader(getter_AddRefs(new mozilla::CSS::Loader()));

As would this.

>+++ b/layout/style/nsCSSLoader.cpp
> class SheetLoadData : public nsIRunnable,
>-		      public nsIUnicharStreamLoaderObserver
>+          public nsIUnicharStreamLoaderObserver

Tabs vs spaces issues here.

>+  SheetLoadData(Loader* aLoader,
>+    const nsSubstring& aTitle,
>+    nsIURI* aURI,
>+    nsICSSStyleSheet* aSheet,
>+    nsIStyleSheetLinkingElement* aOwningElement,
>+    PRBool aIsAlternate,
>+    nsICSSLoaderObserver* aObserver,
>+    nsIPrincipal* aLoaderPrincipal);

And here.

And elsewhere in the SheetLoadData bits here.

>+Loader::Loader(void)
>+  , mRefCnt(1)

As I said, we should init this to 0.  In my opinion.  We could also use nsAutoRefCnt then.

>+Loader::Loader(nsIDocument* aDocument)
>+  , mRefCnt(1)

And here.

>-NS_IMETHODIMP
>+nsresult
> SheetLoadData::OnDetermineCharset(nsIUnicharStreamLoader* aLoader,

I'll be surprised if this compiles on Windows, since it's declared as NS_IMETHOD (stdcall calling convention on Windows) but here implemented with the thiscall calling convention.  This needs to stay NS_IMETHODIMP.

>-NS_IMETHODIMP
>+nsresult
> SheetLoadData::OnStreamComplete(nsIUnicharStreamLoader* aLoader,

Same here.

Is it worth renaming the files to CSSLoader.cpp and CSSLoader.h?

>+++ b/layout/style/nsLayoutStylesheetCache.cpp
>+nsLayoutStylesheetCache::LoadSheet(nsIURI* aURI,
>+    gCSSLoader = new mozilla::CSS::Loader();

If we change the refcounting we need to update this callsite too.

>+++ b/layout/style/test/ParseCSS.cpp
>+    nsRefPtr<mozilla::CSS::Loader>
>+        loader(getter_AddRefs(new mozilla::CSS::Loader());

And here.

r=bzbarsky with the change back to NS_IMETHODIMP on the XPCOM SheetLoadData methods, the refcounting changed to the usual model, and the nits fixed.
Attachment #428481 - Flags: review?(bzbarsky) → review+
Comment on attachment 428480 [details] [diff] [review]
patch v2, part 1: go through nsDocument for speculative and chrome sheet loads

sr=dbaron
Attachment #428480 - Flags: superreview?(dbaron) → superreview+
Comment on attachment 428481 [details] [diff] [review]
patch v2, part 2: deCOM nsCSSLoader

sr=dbaron (given bz's comments being addressed, of course), although I'm wondering if we should really be using uppercase namespaces.  Is it worth trying to keep all namespaces all-lowercase?  (Especially given that we're using "mozilla"?)

I have some code in my tree using mozilla::css...


Sorry for the delay getting to this; I need to get better about distinguishing reviews and superreviews.
Attachment #428481 - Flags: superreview?(dbaron) → superreview+
(In reply to comment #14)
> (From update of attachment 428481 [details] [diff] [review])
> sr=dbaron (given bz's comments being addressed, of course), although I'm
> wondering if we should really be using uppercase namespaces.  Is it worth
> trying to keep all namespaces all-lowercase?  (Especially given that we're
> using "mozilla"?)
> 
> I have some code in my tree using mozilla::css...

I just talked to lw and jwalden, and they both said we've been doing all-lowercase.

So could you switch your namespace CSS to namespace css?
Assignee

Comment 16

9 years ago
(In reply to comment #12)
> (From update of attachment 428481 [details] [diff] [review])
> >+++ b/content/base/src/nsDocument.cpp
> >+  mCSSLoader = new mozilla::CSS::Loader(this);
> 
> This works because the Loader constructor sets mRefCnt to 1, but I'd prefer it
> didn't do that (and that we just NS_ADDREF here).  That would be a lot less
> surprising for callers and hence less leak-prone.

I changed the initial refcount and all the uses.

> > class SheetLoadData : public nsIRunnable,
> >-		      public nsIUnicharStreamLoaderObserver
> >+          public nsIUnicharStreamLoaderObserver
> 
> Tabs vs spaces issues here.

Fixed.  I'm not sure how this happened in the first place.

> >-NS_IMETHODIMP
> >+nsresult
> > SheetLoadData::OnDetermineCharset(nsIUnicharStreamLoader* aLoader,
> 
> I'll be surprised if this compiles on Windows, since it's declared as
> NS_IMETHOD (stdcall calling convention on Windows) but here implemented with
> the thiscall calling convention.  This needs to stay NS_IMETHODIMP.

It did compile on the try server, but I put back the NS_IMETHODIMP.

> Is it worth renaming the files to CSSLoader.cpp and CSSLoader.h?

I don't much care either way; I didn't do it because I didn't know if we have a consensus on doing that.

(In reply to comment #15)
> 
> So could you switch your namespace CSS to namespace css?

Done.

I pushed the revised patches to the try server; I'm not going to land on trunk now because I want to leave shortly and I'll be out of touch all weekend.  Assuming try is happy I'll land on Monday morning.
Comment on attachment 429269 [details] [diff] [review]
v3 part 2: fixes bz and dbaron's comments

This leaks the CSSLoader in nsLayoutStylesheetCache::LoadSheet because it addrefs every time the function is called.  There should just be an NS_IF_ADDREF in the branch that creates the loader.
Attachment #429269 - Flags: review-
Assignee

Comment 20

9 years ago
Thanks for that.  Try server told me something was horribly wrong, but it would have taken me a long time to find.
Assignee

Comment 21

9 years ago
Leak fixed now.
Assignee

Updated

9 years ago
Attachment #428480 - Attachment is obsolete: true
Assignee

Updated

9 years ago
Attachment #428481 - Attachment is obsolete: true
Assignee

Updated

9 years ago
Attachment #429269 - Attachment is obsolete: true
Assignee

Comment 22

9 years ago
http://hg.mozilla.org/mozilla-central/rev/83a2cfa9ac46
http://hg.mozilla.org/mozilla-central/rev/a1f9b5d1ccad
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Comment on attachment 429268 [details] [diff] [review]
v3 part 1: fixes bz's review comments

>+
>+ifndef MOZ_ENABLE_LIBXUL
>+CPP_UNIT_TESTS = TestCSSPropertyLookup.cpp
>+LIBS += ../nsCSSKeywords.o ../nsCSSProps.o $(XPCOM_LIBS)
>+endif
>+

This breaks non-libxul builds on Windows, since Windows object files don't end with .o.
Depends on: 549860
(In reply to comment #23)
>(From update of attachment 429268 [details] [diff] [review])
>>+ifndef MOZ_ENABLE_LIBXUL
>>+CPP_UNIT_TESTS = TestCSSPropertyLookup.cpp
>>+LIBS += ../nsCSSKeywords.o ../nsCSSProps.o $(XPCOM_LIBS)
>>+endif
>>+
> This breaks non-libxul builds on Windows, since Windows object files don't end
> with .o.
It breaks cross-compile builds too ;-)
That should probably use $(OBJ_SUFFIX) instead of "o".
Assignee

Comment 26

9 years ago
(In reply to comment #25)
> That should probably use $(OBJ_SUFFIX) instead of "o".

Standard8 made that change in bug 549860.
You need to log in before you can comment on or make changes to this bug.