Closed Bug 702080 Opened 11 years ago Closed 9 years ago

Audit existing GetDisplayDocument() calls to find ones that should be changed to IsResourceDoc()


(Core :: SVG, defect)

Not set





(Reporter: dholbert, Assigned: Six)





(1 file)

Bug 700895 reminded me that I (or someone) should audit all existing calls to GetDisplayDocument() -- for any of these calls that appear to be as safety-checks for external-resources-documents, we should call IsResourceDoc() instead of GetDisplayDoc(), so that we cover SVG-as-an-image, too.
Taking, so that this doesn't fall off the radar.  (If someone else wants to do this and gets to it before I do, though, feel free to steal it.)
Assignee: nobody → dholbert

Just a question to see if i got it:
we should call IsResourceDoc() when GetDisplayDoc() is called in a conditionnal statement without return assignation?
just like here:

If i'm right a can take it otherwise correct me :)
Flags: needinfo?(dholbert)
Apparently Comment 1 wasn't enough to let me keep this from falling off the radar. :) Thanks for finding & resurrecting it, Arnaud!

To answer your question: it's not as straightforward as that. Some of the GetDisplayDocument() calls are for code that's about to use the display-document (if it exists), and it's just doing a null-check before proceeding. That's the case in the code that you linked to.

It's kind of a case-by-case thing. The cases we're interested in are when GetDisplayDocument() is really a check for things that we don't want to allow helper-documents to do (or things that they don't need to do).  *Those* are the checks where IsResourceDoc() would be appropriate.

From doing a quick scan of an MXR search, the candidate GetDisplayDocument() calls that I think probably want to be converted to IsResourceDoc() are:
and I think that's it.

Want to try converting those ones, give it a try run, and see how it goes?

(bz would know more surely than I whether those ^ cases are appropriate for switching to IsResourceDoc(); he should probably review this.)
Flags: needinfo?(dholbert)
Assigning to Arnaud, since it looks like he's interested in taking a crack at a patch for this. (but Arnaud, feel free to punt it back to me.)
Assignee: dholbert → six.dsn
The frameloaders ones, I think yes.  The nsFrame one, yes if we don't rely on painting inside the resource doc directly for SVG images..
Ok thanks for the details, i will patch it during the week end
should i add reftests to the try run (or any other)?
Probably best to just run all the tests for this, to be on the safe side.  You can keep it to 1 or 2 platforms, though (both debug + opt on each platform) -- if anything's going to break, that'd be likely to flush it out.

(Unlike the MOZ_OVERRIDE bug, this definitely can & should impact behavior, albeit in edge-case ways.)
I changed the lines Daniel has indicate.
Build on Linux64

Tbpl is here:
Attachment #763680 - Flags: review?(bzbarsky)
Comment on attachment 763680 [details] [diff] [review]
replace GetDisplayDocument with IsResourceDoc when needed

This seems fine, though some of the "are we chrome" bits that check GetDisplayDocument might really want to do something magical for svg images....
Attachment #763680 - Flags: review?(bzbarsky) → review+
Ok so are we good for checkin-needed?
Not quite checkin-needed yet -- this hasn't gotten any unit tests.  The Try build in comment 8 just shows builds -- no tests.

(Arnaud: You were unlucky enough to pick two of the platforms where we don't do unit tests, because they're not supported, I think. :)  Or something like that. So "-u all" for those platforms did nothing.)

Could you do one more Try build with, say, linux and macosx64 as the platforms, and all unit tests?
Keywords: checkin-needed
OS: Linux → All
Hardware: x86_64 → All
Ok i'm really unlucky... :)
i will send another Try tomorrow morning
Daniel it seems to be ok, can you confirm and if it's ok set checkin-needed ;)
Flags: needinfo?(dholbert)
Looks good to me!
Flags: needinfo?(dholbert)
Keywords: checkin-needed
This patch landed alongside three other patches <> and caused a b2g mochitest-3 test failure <>, so I backed it out, since it's the only patch that touches something b2g related:

Please watch <> to see if it makes the b2g mochitest-3 suite green again, which would prove that this patch has indeed been at fault.
I'm betting the b2g mochitest-3 failure wasn't from this bug, but was rather from bug 879228 (which landed in the same push).  That bug's patch tweaked Console.jsm, and the M-3 test has this right before it in the mochitest log:
> JavaScript Error: "console is undefined" {file: "resource://gre/modules/ContactDB.jsm" line: 894}]
(and Console.jsm is responsible for exporting the "console" symbol)

I'll watch the tree for a bit, and if this theory pans out and M-3 is still orange, I'll re-land this bug and back out the other one.
Yup, this backout didn't fix it, so this was innocent. I backed out the other bug, and rather than re-landing this on a closed tree (and complicating the already-confusing bustage-upon-bustage state of the tree), I'm going to just mark it as checkin-needed so it can re-land tomorrowish.
Keywords: checkin-needed
Daniel is right.  I relanded the patch:

Sorry for my mistake.
Keywords: checkin-needed
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.