1210459 - Add originAttributes for tests that implement nsILoadContext

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

In Bug 1165466 I added originAttributes into nsILoadContext,
however there are also many test cases implementing nsILoadContext, which still don't have the originAttributes yet, so I'll try to add them in this bug.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch.

Comment 2

[:gerard-majax]

Comment on attachment 8668538 [details] [diff] [review]
Patch.

fixes bug 1210373

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch.

fix more tests that use nsILoadContext

Comment 4

[Henry Chang [:hchang]]

Can we just make up and return an originAttributes based on nsILoadContext.appId/isInBrowserElement/etc when failing to ontain nsILoadContext.originAttribute in [1] instead of fixing all the nsILoadContext instances?

Comment 5

[Henry Chang [:hchang]]

[1] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsILoadContext.idl?offset=100#151

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Henry Chang [:henry] from comment #4)
> Can we just make up and return an originAttributes based on
> nsILoadContext.appId/isInBrowserElement/etc when failing to ontain
> nsILoadContext.originAttribute in [1] instead of fixing all the
> nsILoadContext instances?
That could work, but that seems only fix the crash.
That MOZ_ASSERT was put by Valentin when he took over that bug, 
When I took it back I think it's a good idea to leave the assert there.
Eventually we are going to remove those appId/isInBrowser from nsIXXX and use originAttributes.

Also nsILoadContext could be implemented by JS and cpp code, so if we were going to do that, that means we need to do two JSAPI calls to get appId/isInBrowserElement.

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Bobby's original comment is to return false,  https://bugzilla.mozilla.org/show_bug.cgi?id=1165466#c105, if that is better I could add it back.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/051906f42649

Comment 9

[Honza Bambas (:mayhemer)]

There are much more tests (at least in network) that need this update.  I found a few when fixing bug 1165269 and fighting bug 1165466.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/051906f42649

Comment 11

[Bobby Holley (:bholley)]

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #9)
> There are much more tests (at least in network) that need this update.  I
> found a few when fixing bug 1165269 and fighting bug 1165466.

Now that we have bug 1210508, this should be less of an issue.

Comment 12

[Honza Bambas (:mayhemer)]

(In reply to Bobby Holley (:bholley) from comment #11)
> (In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #9)
> > There are much more tests (at least in network) that need this update.  I
> > found a few when fixing bug 1165269 and fighting bug 1165466.
> 
> Now that we have bug 1210508, this should be less of an issue.

Quite the opposite, I'm afraid.  I had the same code in the last version of my "make it work" patch (bug 1165466) and it caused failures on every call where nsILoadContext JS impl didn't define originAttributes at least as '{}'.  

I think this will be exposed when I turn http cache interfaces to work with OriginAttributes.  And I also think it will be part of that patch, so no worry for you, just a heads up ;)