1411473 - Assertion failure: !mPendingRequest (mPendingRequest should be null.), at /builds/worker/workspace/build/src/dom/base/nsImageLoadingContent.cpp:1407

Status: RESOLVED FIXED

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

Description

[Jason Kratzer [:jkratzer]]

Attachment: trigger.html

Testcase found while fuzzing mozilla-central rev a80d568a417e.

Comment 1

[Jason Kratzer [:jkratzer]]

Attachment: log_minidump.txt

Comment 2

[Jason Kratzer [:jkratzer]]

Attachment: log_stderr.txt

Comment 3

[Ryan VanderMeulen [:RyanVM]]

INFO: Last good revision: ac1520300fc70eddfd93c89cee6ee9a4a95a0142
INFO: First bad revision: a1ac004e0ea91819dd9ef135d13a5de1fb796b82
INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ac1520300fc70eddfd93c89cee6ee9a4a95a0142&tochange=a1ac004e0ea91819dd9ef135d13a5de1fb796b82

Comment 4

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

mCurrentRequest is used to load the data: image, document is https://bug1411473.bmoattachments.org/attachment.cgi?id=8921702

mPendingRequest is used to forcibly reload as referrer-policy is set.
stack:
 #01: nsImageLoadingContent::LoadImage(nsIURI*, bool, bool, nsImageLoadingContent::ImageLoadType, bool, nsIDocument*, unsigned int, nsIPrincipal*) (/home/allstars/src/gecko-dev/dom/base/nsImageLoadingContent.cpp:922)$
 #02: nsImageLoadingContent::ForceReload(mozilla::dom::Optional<bool> const&, mozilla::ErrorResult&) (/home/allstars/src/gecko-dev/dom/base/nsImageLoadingContent.cpp:826)$
 #03: nsImageLoadingContent::ForceReload(bool, unsigned char) (/home/allstars/src/gecko-dev/dom/base/nsImageLoadingContent.cpp:843)$
 #04: nsImageLoadingContent::ForceReload(bool) (/home/allstars/src/gecko-dev/dom/base/nsImageLoadingContent.h:79)$
 #05: mozilla::dom::HTMLImageElement::AfterMaybeChangeAttr(int, nsAtom*, nsAttrValueOrString const&, nsAttrValue const*, nsIPrincipal*, bool, bool) (/home/allstars/src/gecko-dev/dom/html/HTMLImageElement.cpp:500)$
 #06: mozilla::dom::HTMLImageElement::AfterSetAttr(int, nsAtom*, nsAttrValue const*, nsAttrValue const*, nsIPrincipal*, bool) (/home/allstars/src/gecko-dev/dom/html/HTMLImageElement.cpp:326)$
 #07: mozilla::dom::Element::SetAttrAndNotify(int, nsAtom*, nsAtom*, nsAttrValue const*, nsAttrValue&, nsIPrincipal*, unsigned char, bool, bool, bool, nsIDocument*, mozAutoDocUpdate const&) (/home/allstars/src/gecko-dev/dom/base/Element.cpp:2778)$
 #08: mozilla::dom::Element::SetAttr(int, nsAtom*, nsAtom*, nsTSubstring<char16_t> const&, nsIPrincipal*, bool) (/home/allstars/src/gecko-dev/dom/base/Element.cpp:2622)$
 #09: nsIContent::SetAttr(int, nsAtom*, nsAtom*, nsTSubstring<char16_t> const&, bool) (/home/allstars/src/gecko-dev/dom/base/nsIContent.h:380)$
 #10: nsIContent::SetAttr(int, nsAtom*, nsTSubstring<char16_t> const&, bool) (/home/allstars/src/gecko-dev/dom/base/nsIContent.h:376)$
 #11: mozilla::dom::Element::SetAttr(nsAtom*, nsTSubstring<char16_t> const&, mozilla::ErrorResult&) (/home/allstars/src/gecko-dev/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/Element.h:1412)$
 #12: nsGenericHTMLElement::SetHTMLAttr(nsAtom*, nsTSubstring<char16_t> const&, mozilla::ErrorResult&) (/home/allstars/src/gecko-dev/dom/html/nsGenericHTMLElement.h:812)$
 #13: mozilla::dom::HTMLImageElement::SetReferrerPolicy(nsTSubstring<char16_t> const&, mozilla::ErrorResult&) (/home/allstars/src/gecko-dev/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/HTMLImageElement.h:231)$
 #14: mozilla::dom::HTMLImageElementBinding::set_referrerPolicy(JSContext*, JS::Handle<JSObject*>, mozilla::dom::HTMLImageElement*, JSJitSetterCallArgs) (/home/allstars/src/gecko-dev/obj-x86_64-pc-linux-gnu/dom/bindings/HTMLImageElementBinding.cpp:497)


Then the created document (about:blank) tries to load the image.
It bailed out in the aDocument->IsLoadedAsData() part
http://searchfox.org/mozilla-central/rev/5a60492a53667fc61a62af1847d005a210b7a4f6/dom/base/nsImageLoadingContent.cpp#942

Comment 5

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

Hi BZ
I'd like to ask for your suggestion for this case.

There are 3 calls to nsImageLoadingContent::LoadImage

1. triggered by the line ' try { o3.src = 'data:;base64,R0lGODlhAQABAAAAACwAAAAAAQABAAA' } catch(e) { };'
And this sets the mCurrentRequest.

2. triggered by the line ' try { o3.referrerPolicy = 'origin-when-cross-origin' } catch(e) { } '
This forcibly the image to be reloaded, see comment 4 for the call stack.

3. this should be triggered by the document 'document.implementation.createDocument('', '', null)'
And this will go to http://searchfox.org/mozilla-central/source/dom/base/DOMImplementation.cpp#121
and it sets aLoadedAsData to true.

Later when it calls nsImageLoadingContent::LoadImage, It will bail out in the aDocument->IsLoadedAsData() part
http://searchfox.org/mozilla-central/rev/5a60492a53667fc61a62af1847d005a210b7a4f6/dom/base/nsImageLoadingContent.cpp#942

When it calls SetBlockedRequest, at this time we have a non-null mCurrentRequest and a non-null mPendingRequest

I am wondering do you think shoule we move the assert(!mPendingRequest) to the case !HaveSize(mCurrentRequest)  ?
Or we should modify the assertion when the SetBlockedRequest is came from IsLoadedAsData() ?

thanks

Comment 6

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

Attachment: Patch.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> 2. triggered by the line ' try { o3.referrerPolicy = 'origin-when-cross-origin' } catch(e) { } '
> This forcibly the image to be reloaded, see comment 4 for the call stack.

Why?  Per spec changing referrerpolicy should not trigger a new load...

The discussion in bug 1261298, which added this code makes no sense to me.  Per current spec, setting .src and then setting referrer policy should NOT take the referrer policy into account, afaict.  But the web platform tests test for it being taken into account, we have this weird hack to make those tests pass, etc.  What's going on here?

Comment 8

[Josh Matthews [:jdm]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #7)
> > 2. triggered by the line ' try { o3.referrerPolicy = 'origin-when-cross-origin' } catch(e) { } '
> > This forcibly the image to be reloaded, see comment 4 for the call stack.
> 
> Why?  Per spec changing referrerpolicy should not trigger a new load...
> 
> The discussion in bug 1261298, which added this code makes no sense to me. 
> Per current spec, setting .src and then setting referrer policy should NOT
> take the referrer policy into account, afaict.  But the web platform tests
> test for it being taken into account, we have this weird hack to make those
> tests pass, etc.  What's going on here?

The spec says that the actual image request does not start until a stable state is reached (step 1 of https://html.spec.whatwg.org/multipage/images.html#reacting-to-environment-changes), which means that setting the referrerpolicy after src can make a difference.

Comment 9

[Josh Matthews [:jdm]]

Sorry, correct spec link - step 7 of https://html.spec.whatwg.org/multipage/images.html#updating-the-image-data.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Ah, do we have a bug on doing that?  It'll mean moving the "image cache" bits out of imagelib, which would be nice for all sorts of other reasons too...

Comment 11

[Boris Zbarsky [:bzbarsky]]

OK, back to the actual issue in comment 5, the relevant sequence of events is:

1)  Do an image load, wait for it to complete.
2)  Start another image load (this could be done by setting src, not
    referrerPolicy, so the referrerPolicy bit is a bit of a sideshow here).
3)  HTMLImageElement::NodeInfoChanged kicks off yet a third image load (I assume this
    is where the third load comes from).

OK, so #3 is because the adopting steps are a "relevant mutation" for image elements per <https://html.spec.whatwg.org/multipage/images.html#relevant-mutations>.  This should call <https://html.spec.whatwg.org/multipage/images.html#update-the-image-data> and in step 1 we should end up not really doing anything, because the image's node document is not the active document.  This part of the spec is a bit odd; I filed https://github.com/whatwg/html/issues/3211 on it.

In any case, skipping the work NodeInfoChanged() does if the new document isn't supposed to load images (like BindToTree and AfterMaybeChangeAttr do) makes sense to me.

Past that, though, what should happen to an already-loaded image, or one with an in-progress load, is adopted into a document that normally doesn't do image loads.  What do we do (ignoring the assertion)?  What do other browsers do?

Comment 12

[Josh Matthews [:jdm]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #10)
> Ah, do we have a bug on doing that?  It'll mean moving the "image cache"
> bits out of imagelib, which would be nice for all sorts of other reasons
> too...

That's bug 1076583.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

The testcase is overcomplicated.  It's using a sync XHR to wait for the image load, but there is no guarantee that will work, _plus_ it's hacky.  Just wait for the load event on the image.

There's no need for the extra o2 thing either.  Just adopt the image into the data document after the second load is started.

And there's no need to start the second load via the referrerPolicy setter, which shouldn't work per spec _anyway_.  Use a different src value, or set crossOrigin or something.

For the patch itself, I don't think this is the right solution.  See comment 11.

Comment 14

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

Attachment: 1411473.html

Comment 15

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

Attachment: Patch. v2

> Past that, though, what should happen to an already-loaded image, or one
> with an in-progress load, is adopted into a document that normally doesn't
> do image loads.  What do we do (ignoring the assertion)?  What do other
> browsers do?
I might need more time to check spec with this, still keeping the ni?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8926332 [details] [diff] [review]
Patch. v2

> Subject: Bug 1411473 - Skip image loading if OwnerDoc is not allowed to.

"Skip image loading from adoption ...", right?

>http://searchfox.org/mozilla-central/source/dom/base/DOMImplementation.cpp#121

This link will likely be stale by tomorrow.  ;)  I don't think you need to link to the code that makes a document.implementation.createDocument() document a data document; it's clearly a data document.  Just remove this whole paragraph.

>+++ b/dom/base/crashtests/1411473.html

Have you tested that this test fails without the patch?  I mean when run in the harness, not just when loaded directly.

Because I don't see why it would: you don't have a class="reftest-wait" on the root element, so the test would end before we ever finished the image load and hit the assertion...

r=me, but please make sure the test is actually testing things properly.

Comment 17

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

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #16)
> Have you tested that this test fails without the patch?  I mean when run in
> the harness, not just when loaded directly.
> 
I ran this test locally and I did see assertion and failure without my fix, but I'll check the reftest-wait you mentioned again, thanks for your review.

Comment 18

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

Attachment: Patch. v3

addressed bz's comment.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/82622938a7c9
Skip image loading from adoption if OwnerDoc is not allowed to. r=bz

Comment 20

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/82622938a7c9

Comment 21

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

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #11)
> Past that, though, what should happen to an already-loaded image, or one
> with an in-progress load, is adopted into a document that normally doesn't
> do image loads.  What do we do (ignoring the assertion)?  What do other
> browsers do?
I think the that's the 

For Chromium
when adoption happens, HTMLImageElement::DidMoveToNewDocument will be called
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Source/core/html/HTMLImageElement.cpp#601

The first line 'SelectSourceURL(ImageLoader::kUpdateIgnorePreviousError);' is an no-op, as the GetDocument().IsActive() is false so it bailed out immediately.

Then it calls ImageLoader::ElementDidMoveToNewDocument 
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Source/core/loader/ImageLoader.cpp#774

this will call ImageLoader::SetImageWithoutConsideringPendingLoadEvent in turn, 
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Source/core/loader/ImageLoader.cpp#270

which clears all pending error/load event, and call UpdateImageState(nullptr).

This should match the spec Step 15.1 in https://html.spec.whatwg.org/multipage/images.html#reacting-to-environment-changes

    "If the img element has experienced relevant mutations since this algorithm started, then let pending request be null and abort these steps."

However in our mozilla-central, we don't cancel pending requests at the moment.


For the 'current request', or alreay-loaded image, whether it's adopted or not,
I think it's step 1 in  https://html.spec.whatwg.org/multipage/images.html#updating-the-image-data
"If the element's node document is not the active document, then:...."

In ImageLoader.cpp and HTMLImageElement.cpp from Chromium's codebase, they check Document()->IsActive() in many places, so for example if this is data document and it's IsActive() will be false, so many functions will bail out early.

Comment 22

[Boris Zbarsky [:bzbarsky]]

> For Chromium

Thank you, but I meant what is the actual observable behavior.  Once adopted into the data document, does the image still have its image data?  Does it have a naturalWidth/naturalHeight that show that data?  Can it be drawn into a canvas with drawImage?  That sort of thing.  And across all browsers, not just Chromium.

Put another way, we should have web platform tests for all this stuff...

Comment 23

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

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #22)
> Thank you, but I meant what is the actual observable behavior.  Once adopted
> into the data document, does the image still have its image data?  Does it
> have a naturalWidth/naturalHeight that show that data?  Can it be drawn into
> a canvas with drawImage?  That sort of thing.  And across all browsers, not
> just Chromium.
> 
Ah, sorry.

see http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=5501

Firefox/Edge/IE still keeps the width/height and can be drawn into canvas.
Whereas Safari and Chrome reset the image data, width/height are both 0 and the canvas is empty.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Alright.  Do you mind filing an HTML spec bug to figure out what the right behavior is in this situation?

Comment 25

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

Sure, https://github.com/whatwg/html/issues/3226
Just filed my first bug in HTML spec. \O/