1016875 - URL that returns a non-text/html type with HTML Imports causes crash

Status: RESOLVED FIXED

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

Description

[Frederik Braun [:freddy]]

Attachment: output.txt

When supplying a view-source URL as import source, Nightly just crashes.

The test case is simple enough to fit in a data URI, just copy&paste into a Nightly build with the webcomponents pref enabled:

data:text/html,<link id="viewSourceWithCORSDisabled" rel="import" href="view-source:http://example.com">

I have also run this in an address sanitizer build. The console output is attached as output.txt

Comment 1

[Frederik Braun [:freddy]]

As a recommendation on how to fix this: Can we please just disallow all non-HTTP/HTTPs schemes? :)
I'm not sure how far the recommendation goes with data URIs and Blob URIs though...

Comment 2

[Frederik Braun [:freddy]]

FWIW I get the same error with ftp:, resource:, file:, jar: and about: URIs.

Comment 3

[Robert Kaiser]

bp-e46cb74b-27b8-4d3d-9d4b-34db52140528 is not a signature, but a report. :)

Comment 4

[Frederik Braun [:freddy]]

I also get a crash when I import a gif.
Possibly the same bug (crash report at https://crash-stats.mozilla.com/report/index/f8d01afa-f16a-42fe-94f4-2950c2140528)?

Comment 5

[Daniel Veditz [:dveditz]]

The crashes are null derefs so probably not exploitable.

We should not allow loading URLs that are not allowed in an iframe src= at the very least and we should enforce that the thing we loaded had a content-type of text/html -- no sniffing allowed in new specs! If we're crashing because we've already flagged these things as errors then great; I only bring it up in case fixing the crash means that we'd then load these invalid things.

Comment 6

[Frederik Braun [:freddy]]

This is for HTML Imports (webcomponents) a new feature, not iframes. So I guess this is not a regression?

Comment 7

[Boris Zbarsky [:bzbarsky]]

It's a regression in the sense that a web page that used to not crash now crashes.  The only good thing is this is preffed off by default.  ;)

The handling of ImportLoader::mChannel looks totally bogus to me.  For example, it completely ignores the possibility of HTTP redirects, which would cause the request passed to the callbacks to not match what NS_NewChannel returned.  That makes debugging the rest of what happens here extra fun.

The actual bug here is that the code in OnStopRequest is assuming mDocument is not null, aka that the load didn't fail.  There is no particular reason that should be the case.  In fact, OnStartRequest fails out the load if the type is not text/html, which is what causes the crash in this bug: OnStartRequest fails the load and doesn't create a document, OnStopRequest doesn't check the load status and tries to use the document.

As long as we're here:

1)  There is no checking for HTTP error pages (should those still be loaded as the
    import?).
2)  Why does the import document share the main document's loadgroup instead of using its
    own that's placed in the import parent's loadgroup?  Maybe this is sensible, but just
    want to make sure that was a conscious decision.
3)  Why does ImportLoader CC mDocument but not mImportParent?

In general, I assume you did read over the existing code in nsExternalResourceMap::PendingLoad that does similar-but-not-identical things while writing this, right?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Incidentally that means we have no tests for the non-text/html behavior.

Comment 9

[Gabor Krizsanits (INACTIVE)]

Just to make this clear, this is just an initial state for the imports, there are two reasons for it to be on mc, one is to save it from bit-rotting the other is to let some GAIA folks play with it... Anyway, these are are great points so I'm very thank full for looking into this.

(In reply to Boris Zbarsky [:bz] from comment #7)
> 1)  There is no checking for HTTP error pages (should those still be loaded
> as the
>     import?).

No, that part is to be implemented. That part in general feels a bit underspecified to me...
But maybe I just looked over something, in any case, it has to be worked out and implemented.

> 2)  Why does the import document share the main document's loadgroup instead
> of using its
>     own that's placed in the import parent's loadgroup?  Maybe this is
> sensible, but just
>     want to make sure that was a conscious decision.

Semi-conscious... an import can have multiple parents in which case it's hard to tell in which group it should be placed... I might be wrong about this, so it's a very valid question, I just _feel_ like this is the right thing to do which gives me very little comfort... What would be the advantage/disadvantage of not sharing the main document's loadgroup exactly? 

> 3)  Why does ImportLoader CC mDocument but not mImportParent?

mImportParent is just set temporarily until the import is loaded (or failed), then it's nulled out. We _could_ CC it but we decided it's not necessary. If I have not added a comment about it I should. 
(actually I just recalled it, it was in the CC list with a comment, and then when I removed it the comment went with it... will add back the comment at least)

> 
> In general, I assume you did read over the existing code in
> nsExternalResourceMap::PendingLoad that does similar-but-not-identical
> things while writing this, right?

Yes, it's kind of a mixture of that and XHR... And I'm sure a lot of important things are still missing. I barely made the right case scenario to work, there are a lot of polishing needed around the import loading.

(In reply to Boris Zbarsky [:bz] from comment #8)
> Incidentally that means we have no tests for the non-text/html behavior.

This is the best sum of the current case in general. It's time to write a bunch of tests, and the hardest part about it to me is to find a good list of things that should be tested. So this bug is already incredibly useful for me.

Comment 10

[Gabor Krizsanits (INACTIVE)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #9)
> an import can have multiple parents

Just to clarify this part a bit, mImportParent points to the parent that should matter in the script execution order calculation, and it might change as more link imports found by the parsers referring to the same import.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> in which case it's hard to tell in which group it should be placed

The group of the main document, obviously.  ;)

> What would be the advantage/disadvantage of not sharing the main document's loadgroup
> exactly? 

One advantage would be the ability to stop loads for all of an import's subresources without stopping loads for the rest of the subresources for the main document.

Another is that addons won't grab a docshell from your loadgroup and assume you're the document rendering in that docshell; that was the motivation behind the separate loadgroups for SVG resource documents.

> mImportParent is just set temporarily until the import is loaded (or failed)

If that's the intent, it's not implemented correctly.  For example, if AsyncOpen() on the channel fails you will hold on to it forever (as well as blocking scripts forever, actually).  Also, it relies on DOMContentLoaded firing, and it's not clear to me that anything 100% guarantees that.  I'd be less worried if we fixed the AsyncOpen issue and explicitly did cleanup in OnStopRequest, since that's the one thing we _are_ guaranteed will happen if AsyncOpen succeeded.  At least modulo broken extension protocol implementations.

Comment 12

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky [:bz] from comment #11)
> > in which case it's hard to tell in which group it should be placed
> 
> The group of the main document, obviously.  ;)

Right so you mean creating loadGroups for each import but add them all to the masters? Because at the beginning we have no info about the import graph... So either go with the idea (everything to the masters group approach), or we have to be able to move loadGroups around when it turns out that there is another parent document for it then the one that initiated its loading...

Anyway, stopping the import's subresources shouldn't be needed according to Anne. Currently imports are defined as external resources of the main document pretty much, so I kind of agree with him.

The addon's grabbing a docshell part is... I honestly have no idea.

> > mImportParent is just set temporarily until the import is loaded (or failed)
> 
> If that's the intent, it's not implemented correctly.  For example, if
> AsyncOpen() on the channel fails you will hold on to it forever (as well as
> blocking scripts forever, actually).

Will fix that.

>  Also, it relies on DOMContentLoaded
> firing, and it's not clear to me that anything 100% guarantees that.  I'd be
> less worried if we fixed the AsyncOpen issue and explicitly did cleanup in
> OnStopRequest, since that's the one thing we _are_ guaranteed will happen if
> AsyncOpen succeeded.  At least modulo broken extension protocol
> implementations.

Thank you, I will do that. About the event firing, the idea is that I add a timeout
like in XHR. The spec is super vague about this part, and leaves it up to UA to decide
what to do. I'm not big fan of timeouts but don't have any better idea either... :(

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Right so you mean creating loadGroups for each import but add them all to the masters? 

Yes, that's the only viable alternative to just putting everything directly in the master loadgroup.

> Currently imports are defined as external resources of the main document pretty much

OK.  And we don't fire separate observable load events on the import documents themselves or anything, so don't need to track their subresource loads separately?

> I'm not big fan of timeouts but don't have any better idea either

I wouldn't worry about it.

Comment 14

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky [:bz] from comment #13)
> OK.  And we don't fire separate observable load events on the import
> documents themselves or anything, so don't need to track their subresource
> loads separately?
> 

"The link element fires a simple event called load for successful loading attempt. "- what we have in the spec... We actually fire this event after the import document's DOMContentLoaded event, which might be not the right thing to do, now that I think about it... I have never put a lot of thoughts about images and such in an import document since we don't render them anyway, stylesheets can be important though and maybe iframes too... Load event on the import document itself, is actually a very good question, I _think_ we have to fire them. Does that mean they need their own loadGroup?

Comment 15

[Boris Zbarsky [:bzbarsky]]

If you need to separately keep track of the loading state of the import document and its subresources, then you want them to have a separate loadgroup to track that state.  How are you going to track it otherwise?

Comment 16

[Gabor Krizsanits (INACTIVE)]

So I'm trying to figure out how to do the redirection part correctly. Other than stop assuming that in the callback I get the same request that I store in mChannel, do I need to do any specific checks? Just to clarify I'm trying to follow redirections as http://fetch.spec.whatwg.org/#atomic-http-redirect-handling

Also, this w3c bug should be linked here I think: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25924

Comment 17

[Boris Zbarsky [:bzbarsky]]

> do I need to do any specific checks?

At that point, can't you just stop storing mChannel altogether?

Comment 18

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky [:bz] from comment #17)
> At that point, can't you just stop storing mChannel altogether?

Yepp, that's my intention :) Just don't know how cautious should I be with calling SetOwner on it... (other than checking for error after call, which I seem to forgot to do in my patch)

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Just don't know how cautious should I be with calling SetOwner on it...

You want to call SetOwner() on the actual channel that the resulting document will use, no?

Comment 20

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky [:bz] from comment #19)
> You want to call SetOwner() on the actual channel that the resulting
> document will use, no?

Yes, for sure. My worry might be totally meaningless, I was just reading some code related to redirection/channels today and found this: http://mxr.mozilla.org/mozilla-central/source/content/base/src/EventSource.cpp#374

I'm not sure what can be the result of giving a channel a system principal, and if we should do some guarding for this case or not...

Comment 21

[Boris Zbarsky [:bzbarsky]]

> I'm not sure what can be the result of giving a channel a system principal

The resulting document has the system principal.

> and if we should do some guarding for this case or not

Good question.  What should happen with imports in system pages?

Comment 22

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky [:bz] from comment #21)
> Good question.  What should happen with imports in system pages?

In that case I think the only sane thing we could do is to prevent chrome documents to import anything from non-chrome locations... Which I'm not sure if easy to do. We have to be able to tell if an URI is chrome, I'm sure we have some API for that... and then I _think_ it's enough to check it at the very beginning if the to-be-imported URI is chrome or not. Or is there any situation where we might have to deal with redirection or any other tricks in the chrome case? I think I will have to prevent redirection to data uris anyway, so I will have to check if redirection has occurred or not, I might as well add an extra check to prevent redirection entirely in the chrome case just in case...

Comment 23

[Boris Zbarsky [:bzbarsky]]

> Which I'm not sure if easy to do.

Stepping back for a sec, what do we do with chrome that has <script src="somewhere">? Seems like the threat model here is exactly the same.

Comment 24

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky [:bz] from comment #23)
> > Which I'm not sure if easy to do.
> 
> Stepping back for a sec, what do we do with chrome that has <script
> src="somewhere">? Seems like the threat model here is exactly the same.

Hmmm... I will look into it, it's a good starting point, thanks. I have to go now, so I will get back to this tomorrow.

Comment 25

[Gabor Krizsanits (INACTIVE)]

Interesting. The worker script loader does something similar what I was planning to do: http://mxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#533

But the regular script element behavior depends on the CORS check: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsScriptLoader.cpp#1493

if there was no CORS check we seem like running the script with the channels principal. If there was CORS check (which should be the case for imports...) then it seems like we do nothing.

Import documents share the browser context of the master, so we cannot just import a non-chrome document, I feel like we have to do something like the worker code does.

Comment 26

[Gabor Krizsanits (INACTIVE)]

I could use some help with channel redirection...

I think I will have to implement nsIChannelEventSink::asyncOnChannelRedirect to be able to cancel some redirections (it's not yet clear what to cancel, but redirection from some site to a file on my hard drive probably should not be allowed for example...)

What I don't know is how the redirection happen exactly (without asyncOnChannelRedirect for now). I async open a channel and waiting for OnStartRequest/OnStopRequest/OnDataAvailable calls. IF the redirection occurs, will I get these calls only with the new (post redirection) channel, or some of them will be called with the old one (the channel I opened) too? Or only with the old one and for the new channel to call these methods I have to do some registration in the asyncOnChannelRedirect? (it does not seem like it, but I could have missed something)

Boris, could you help me out or point me to some code / people I should bother with my questions?

Comment 27

[Boris Zbarsky [:bzbarsky]]

> IF the redirection occurs, will I get these calls only with the new (post redirection)
> channel

Yes.  Note that this is actually documented in nsIChannel.idl in the docs for asyncOpen.

I'm happy to answer necko questions in general; Honza Bambas (:mayhemer) is the expert on redirection stuff if you end up having to dig deep into it for some reason.

Comment 28

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky [:bz] from comment #27)
> Yes.  Note that this is actually documented in nsIChannel.idl in the docs
> for asyncOpen.

Exactly what I was looking for :) Thanks!

Comment 29

[Gabor Krizsanits (INACTIVE)]

Attachment: Part1: Fixing leaking of mImportParent. v1

Just to be future proof I added mImportParent to the CC list. It is too simple to make a mistake and end up leaking mImportParent if we don't.

Comment 30

[Gabor Krizsanits (INACTIVE)]

Attachment: part2: timeout for imports. v1

The default timeout is a question... since the spec does not give any guideline I wouldn't spend a lot of time on it for now. Later this part should be polished probably, the number should depend on the subimports OR there should be an API for changing it like for XHR, but then it should be done on spec level.
Also this is a slow test :(
mChannel cannot be removed because we need to cancel it during timeout, but it is only used for there (and in Open but that should not be an issue)

Comment 31

[Gabor Krizsanits (INACTIVE)]

Attachment: part3: Redirection support. v1

I'm not sure if this patch is review ready... it's kind of a best bet from my side, am I missing anything?

Comment 32

[Gabor Krizsanits (INACTIVE)]

Attachment: part4: Own LoadGroup for each import. v1

Comment 33

[Gabor Krizsanits (INACTIVE)]

Attachment: part5: test for blob and data imports

Comment 34

[Gabor Krizsanits (INACTIVE)]

Attachment: part6: System documents should not import non-system documents. v1

Comment 35

[Gabor Krizsanits (INACTIVE)]

Attachment: part7: GetPrincipal for imports. v1

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8448795 [details] [diff] [review]
part2: timeout for imports. v1

Why do we want a timeout by default?  This seems like it would make any tests involving imports the stuff of sheriffs' nightmares....

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8448797 [details] [diff] [review]
part3: Redirection support. v1

How about just passing "this" to NS_NewChannel like you're supposed to if you know the callbacks up front? ;)  Then you don't need mNotificationCallbacks either.

> +  // XXX: should we call CheckMayLoad here?

Aren't you supposed to be using CORS?  I don't see it anywhere here, but if you were it would handle that for you, no?  It's a bit more complicated than just CheckMayLoad.

>\ No newline at end of file

Several of those in the patch; should fix.

Comment 38

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky [:bz] from comment #36)
> Comment on attachment 8448795 [details] [diff] [review]
> part2: timeout for imports. v1
> 
> Why do we want a timeout by default?  This seems like it would make any
> tests involving imports the stuff of sheriffs' nightmares....

Yeah... That's a totally valid point, just I don't know what else can we do? If a site has
a link import (maybe from another server) and it turns out to be hanging, the script of the
site is blocked until the import is finished loading (and executing its scripts) which might
never happen... and the site won't have any chance to recover. My point is that never giving up on
an imports is bad too and I have no idea how to address the problem. I'm open for suggestions what should we do...

(In reply to Boris Zbarsky [:bz] from comment #37)
> Comment on attachment 8448797 [details] [diff] [review]
> part3: Redirection support. v1
> 
> How about just passing "this" to NS_NewChannel like you're supposed to if
> you know the callbacks up front? ;)  Then you don't need
> mNotificationCallbacks either.
> 
> > +  // XXX: should we call CheckMayLoad here?
> 
> Aren't you supposed to be using CORS?  I don't see it anywhere here, but if
> you were it would handle that for you, no?  It's a bit more complicated than
> just CheckMayLoad.
> 

Yes I should do that... so here is how I think this should look like based on this comment
and some code I read (I might be completely wrong...):
- creating a channel with |this| (not sure if |this| is needed if I use the CORSListenerProxy)
- creating a nsCORSListenerProxy with |this| and then Init it with channel
- AsyncOpen with the CORSListenerProxy
- after this I only have to call the nsIAsyncVerifyRedirectCallback argument in AsyncOnChannelRedirect
the rest should be taken care for me by the proxy

In this case maybe I should use some helper class instead of implementing nsIChannelEventSink

Comment 39

[Boris Zbarsky [:bzbarsky]]

> If a site has a link import (maybe from another server) and it turns out to be hanging,
> the script of the site is blocked until the import is finished loading (and executing its
> scripts) which might never happen

How is that different from the <script> situation?

> - creating a channel with |this| (not sure if |this| is needed if I use the
> CORSListenerProxy)

It is.

> after this I only have to call the nsIAsyncVerifyRedirectCallback argument in
> AsyncOnChannelRedirect

And update mChannel, no?

Comment 40

[Boris Zbarsky [:bzbarsky]]

That's assuming you still want an mChannel, of course.

Comment 41

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky [:bz] from comment #39)
> How is that different from the <script> situation?

Maybe it will be a more common scenario than with scripts... Importing a UI element from an external place. But conceptually nothing, so probably this is only a problem in my mind then, I can just throw away the whole timeout idea.

(In reply to Boris Zbarsky [:bz] from comment #40)
> That's assuming you still want an mChannel, of course.

I think I can get rid of mChannel if we don't need timeout.

Comment 42

[Boris Zbarsky [:bzbarsky]]

> Importing a UI element from an external place

People use <script> for that now.

> I think I can get rid of mChannel if we don't need timeout.

Let's do that.  Then you don't need to implement nsIChannelEventSink or nsIInterfaceRequestor and don't need to set yourself as the callbacks on the channel.

Comment 43

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8448795 [details] [diff] [review]
part2: timeout for imports. v1

Review of attachment 8448795 [details] [diff] [review]:
-----------------------------------------------------------------

As bz pointed out, we shouldn't do the automatic timeout thing, but there were a couple of hunks that seemed unrelated to the patch as a whole (and one that definitely needs to be landed).

::: content/base/src/ImportManager.cpp
@@ +134,5 @@
>      return nsContentUtils::DispatchTrustedEvent(mNode->OwnerDoc(),
>                                                  mNode,
>                                                  mSuccess ? NS_LITERAL_STRING("load")
>                                                           : NS_LITERAL_STRING("error"),
> +                                                /* aCanBubble = */ false,

This change seems misplaced in this patch. Why make it?

@@ +312,5 @@
>  
> +  if (!mDocument) {
> +    // If at this point we don't have a document, then the error was
> +    // already reported.
> +    return NS_ERROR_FAILURE;

This hunk should live on and get landed (see bug 1033443).

Comment 44

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8448803 [details] [diff] [review]
part5: test for blob and data imports

Review of attachment 8448803 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/test/test_imports_nonhttp.html
@@ +16,5 @@
> +  //<![CDATA[
> +    SimpleTest.waitForExplicitFinish();
> +    var counter = 0;
> +    function loaded() {
> +      if ( counter++ == 1 ) {

Odd spacing.

@@ +36,5 @@
> +    var encoded = btoa(stringDoc);
> +    var dataurl = "data:text/html;base64," + encoded;
> +    link.setAttribute("href", dataurl);
> +    link.setAttribute("onload", "loaded()");
> +    link.setAttribute("onerror", "failed()");

Why not set link.onload directly or use addEventListener?

Comment 45

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8448805 [details] [diff] [review]
part6: System documents should not import non-system documents. v1

Review of attachment 8448805 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/ImportManager.cpp
@@ +346,5 @@
>      return NS_ERROR_FAILURE;
>    }
>  
>    nsCOMPtr<nsIPrincipal> principal = sop->GetPrincipal();
> +  if (nsContentUtils::IsSystemPrincipal(principal)) {

Shouldn't this be a call to nsIScriptSecurityManager::CheckURIWithPrincipal, similar to <http://mxr.mozilla.org/mozilla-central/source/layout/style/Loader.cpp?rev=ed5afdaaa6d3#1013>?

Comment 46

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8448797 [details] [diff] [review]
part3: Redirection support. v1

Review of attachment 8448797 [details] [diff] [review]:
-----------------------------------------------------------------

If I'm reading the bug correctly, there's a new patch coming here, right?

Comment 47

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8448808 [details] [diff] [review]
part7: GetPrincipal for imports. v1

Review of attachment 8448808 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the weak ref (and corresponding changes -- we don't need to keep refcounting everywhere.

::: content/base/src/ImportManager.h
@@ +123,5 @@
>    // on the import parent document.
>    void BlockScripts();
>    void UnblockScripts();
>  
> +  already_AddRefed<nsIPrincipal> GetPrincipal();

This can return a weak ref.

Comment 48

[Boris Zbarsky [:bzbarsky]]

> Shouldn't this be a call to nsIScriptSecurityManager::CheckURIWithPrincipal,

No, though we need that too, in Open()!

Comment 49

[Gabor Krizsanits (INACTIVE)]

(In reply to Blake Kaplan (:mrbkap) from comment #43)
> >      return nsContentUtils::DispatchTrustedEvent(mNode->OwnerDoc(),
> >                                                  mNode,
> >                                                  mSuccess ? NS_LITERAL_STRING("load")
> >                                                           : NS_LITERAL_STRING("error"),
> > +                                                /* aCanBubble = */ false,
> 
> This change seems misplaced in this patch. Why make it?

Because otherwise if an error event is dispatched and the callback does not cancel it explicitly test will break (since the even bubbles up further). I tried to find any guidelines when should an event bubble and when not, but couldn't... So I would really like to hear some input about this. Spec only say "simple event" does that specify it?  

> 
> @@ +312,5 @@
> >  
> > +  if (!mDocument) {
> > +    // If at this point we don't have a document, then the error was
> > +    // already reported.
> > +    return NS_ERROR_FAILURE;
> 
> This hunk should live on and get landed (see bug 1033443).

I will add this as a separate patch and land it with r=you then. Sorry for the lag here, I had an unexpected PTO because I didn't have my notebook charger with me in Hungary...

Comment 50

[Boris Zbarsky [:bzbarsky]]

> Spec only say "simple event" does that specify it?  

See http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#fire-a-simple-event

Comment 51

[Gabor Krizsanits (INACTIVE)]

Attachment: Part2: Ensure mDocument in ImportLoader::OnStopRequest. v1

Probably I could just flag this r+ but better safe than sorry...

Comment 52

[Gabor Krizsanits (INACTIVE)]

Attachment: Part3: Events in HTML imports should not bubble. v1

Per Comment 50

Comment 53

[Gabor Krizsanits (INACTIVE)]

Attachment: Part4: Removing mChannel in ImportLoader. v1

Comment 54

[Gabor Krizsanits (INACTIVE)]

Attachment: Part5: Redirection test for Imports. v2

Comment 55

[Gabor Krizsanits (INACTIVE)]

Attachment: Part6: CORS and other security checks for imports. v1

Comment 56

[Gabor Krizsanits (INACTIVE)]

Attachment: Part7: Imports should have their own LoadGroup

It's just a rebased version.

Comment 57

[Gabor Krizsanits (INACTIVE)]

Attachment: part9: System should only import other system documents. v2

I think we still need this patch...

Comment 58

[Gabor Krizsanits (INACTIVE)]

Attachment: Part10: GetPrincipal for imports. v2

Updated version of the already r+ one.

Comment 59

[Gabor Krizsanits (INACTIVE)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #55)
> Created attachment 8456202 [details] [diff] [review]
> Part6: CORS and other security checks for imports. v1

Weird... the CORS part seems to break data URIs for some reason... regular URI's and even blob works fine. Is there a reason why nsCrossSiteListenerProxy should not work when the channel belongs to a dataURI? I get things like this on my console:

247	  rv = corsListener->Init(channel);
(gdb) n
[4151] WARNING: CacheIndex::SetupDirectoryEnumerator() - Entries directory doesn't exist!: file /home/gabor/development/imports/netwerk/cache2/CacheIndex.cpp, line 2515
ERROR: GetDiskSpaceAvailable: STATFS call FAILED. 
[4151] WARNING: 'NS_FAILED(rv)', file /home/gabor/development/imports/netwerk/cache2/CacheFileIOManager.cpp, line 2575
[4151] WARNING: NS_ENSURE_TRUE(http) failed: file /home/gabor/development/imports/content/base/src/nsCrossSiteListenerProxy.cpp, line 859
248	  NS_ENSURE_SUCCESS_VOID(rv);

Comment 60

[Boris Zbarsky [:bzbarsky]]

>+  rv = corsListener->Init(channel);

Might want to pass "true" for the optional aAllowDataURI argument to solve you issue from comment 59.  Also, boo optional boolean arguments.

Comment 61

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8456202 [details] [diff] [review]
Part6: CORS and other security checks for imports. v1

Why ALLOW_CHROME for the CheckLoadURI call?  I don't think we want that.

Apart from that and the data: thing, looks reasonable.

Comment 62

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8456198 [details] [diff] [review]
Part4: Removing mChannel in ImportLoader. v1

Review of attachment 8456198 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/ImportManager.cpp
@@ +258,1 @@
>                                                         aStream, aOffset,

This indentation looks odd to me.

Comment 63

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8456202 [details] [diff] [review]
Part6: CORS and other security checks for imports. v1

Review of attachment 8456202 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with bz's comments addressed.

Comment 64

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky [:bz] from comment #61)
> Why ALLOW_CHROME for the CheckLoadURI call?  I don't think we want that.

For sure not... :( sorry about that I forgot to update the patch before filing it...

https://tbpl.mozilla.org/?tree=Try&rev=17295d6306ee

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/98c0de173604
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6c958a605ac1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c7d87f38aeef
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bbc328176472
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/06a6bfac90ae
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c1a411690bdb
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4d96a938b6ff
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/506cbe3ed5e9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3b9951da1581
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ffa4255ebaa7

Comment 65

[Carsten Book [:Tomcat]]

sorry had to backout the last part for bustage on linux like https://tbpl.mozilla.org/php/getParsedLog.php?id=43929466&tree=Mozilla-Inbound

Comment 66

[Gabor Krizsanits (INACTIVE)]

(In reply to Carsten Book [:Tomcat] from comment #65)
> sorry had to backout the last part for bustage on linux like
> https://tbpl.mozilla.org/php/getParsedLog.php?id=43929466&tree=Mozilla-
> Inbound

Thanks for your help! I did a full try run this time: https://tbpl.mozilla.org/?tree=Try&rev=8a0860f22b9d

Comment 67

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/98c0de173604
https://hg.mozilla.org/mozilla-central/rev/6c958a605ac1
https://hg.mozilla.org/mozilla-central/rev/c7d87f38aeef
https://hg.mozilla.org/mozilla-central/rev/bbc328176472
https://hg.mozilla.org/mozilla-central/rev/06a6bfac90ae
https://hg.mozilla.org/mozilla-central/rev/c1a411690bdb
https://hg.mozilla.org/mozilla-central/rev/4d96a938b6ff
https://hg.mozilla.org/mozilla-central/rev/506cbe3ed5e9
https://hg.mozilla.org/mozilla-central/rev/3b9951da1581

Comment 68

[Gabor Krizsanits (INACTIVE)]

The backed out last patch re-push:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4aa73f5db7b5

Comment 69

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4aa73f5db7b5